diff --git a/cmd/hull/prune.go b/cmd/hull/prune.go index 72ad357..76f23fd 100644 --- a/cmd/hull/prune.go +++ b/cmd/hull/prune.go @@ -21,14 +21,9 @@ import ( "errors" "fmt" "io" - "io/fs" "os" "path/filepath" "sort" - "strconv" - "strings" - "syscall" - "time" "github.com/urfave/cli/v3" @@ -36,20 +31,6 @@ import ( "github.com/brig-sh/hull/pkg/store" ) -// pullStagingGrace is how long a staging directory whose pid is still alive is -// left alone. -// -// A `.tmp-` belonging to a pull in flight must not be deleted: -// doing so fails that pull, and this command is expected to be safe to run at -// any time. But a pid is weak evidence -- the kernel recycles pid numbers, so -// an unrelated process can inherit the number of the hull that left the -// directory behind, and the entry would then be immortal. -// -// So the pid only protects a directory that was also touched recently. No pull -// takes an hour, and a directory older than that belongs to a hull that is -// long gone whatever is answering to its pid now. -const pullStagingGrace = time.Hour - func pruneCommand() *cli.Command { return &cli.Command{ Name: "prune", @@ -58,8 +39,8 @@ func pruneCommand() *cli.Command { "left by an interrupted pull, image directories whose metadata or rootfs is " + "missing, and digests superseded by a later pull of the same reference and " + "platform. With --all it also removes every image no instance refers to.\n\n" + - "An image any instance refers to is never removed, running or stopped. Use " + - "`hull rmi --force` for that case.\n\n" + + "An image any instance refers to is never removed, running or stopped. Stop " + + "running instances first, then use `hull rmi --force` for that case.\n\n" + "Nothing removed here is unrecoverable: every image can be pulled again.\n\n" + "Deleting inside the store does not shrink its backing sparse image. Run " + "`hull store compact` afterwards to return the space to the host.", @@ -107,7 +88,7 @@ type pruneTarget struct { } func pruneStoreIn(w io.Writer, s *store.Store, all, dryRun bool, compactHint string) error { - targets, err := pruneTargets(s, all, time.Now()) + targets, err := pruneTargets(s, all) if err != nil { return err } @@ -116,6 +97,25 @@ func pruneStoreIn(w io.Writer, s *store.Store, all, dryRun bool, compactHint str return nil } + reclaimed, err := removePruneTargets(w, s, targets, dryRun) + + verb := "Reclaimed" + if dryRun { + verb = "Would reclaim" + } + _, _ = fmt.Fprintf(w, "%s %s\n", verb, formatSize(reclaimed)) + // Not after a dry run: nothing has been freed yet, so pointing at + // compaction there would be advice to compact around what is still in the + // store. + if compactHint != "" && reclaimed > 0 && !dryRun { + _, _ = fmt.Fprintln(w, compactHint) + } + return err +} + +// removePruneTargets removes (or, on a dry run, reports) each target, and +// returns the bytes reclaimed along with every removal that failed. +func removePruneTargets(w io.Writer, s *store.Store, targets []pruneTarget, dryRun bool) (int64, error) { var reclaimed int64 var failures []error for _, t := range targets { @@ -126,10 +126,23 @@ func pruneStoreIn(w io.Writer, s *store.Store, all, dryRun bool, compactHint str } var rmErr error if t.staging { - rmErr = s.RemoveStagingDir(t.name) + var got bool + got, rmErr = s.RemoveStagingDir(t.name) + if rmErr == nil && !got { + // A pull's sweep took it between the listing and now. The + // space is free, but not by this run, so it is not counted. + _, _ = fmt.Fprintf(w, "Already gone: %s (%s)\n", t.describe(), t.reason) + continue + } } else { rmErr = s.DeleteImage(t.name) } + if errors.Is(rmErr, store.ErrStagingInUse) { + // A pull started using it between the listing and now. Not a + // failure: it is exactly what the listing would have skipped. + _, _ = fmt.Fprintf(w, "Left %s alone (a pull is using it)\n", t.describe()) + continue + } if rmErr != nil { failures = append(failures, rmErr) continue @@ -137,19 +150,7 @@ func pruneStoreIn(w io.Writer, s *store.Store, all, dryRun bool, compactHint str _, _ = fmt.Fprintf(w, "Removed %s (%s, %s)\n", t.describe(), t.reason, formatSize(t.bytes)) reclaimed += t.bytes } - - verb := "Reclaimed" - if dryRun { - verb = "Would reclaim" - } - _, _ = fmt.Fprintf(w, "%s %s\n", verb, formatSize(reclaimed)) - // Not after a dry run: nothing has been freed yet, so pointing at - // compaction there would be advice to compact around what is still in the - // store. - if compactHint != "" && reclaimed > 0 && !dryRun { - _, _ = fmt.Fprintln(w, compactHint) - } - return errors.Join(failures...) + return reclaimed, errors.Join(failures...) } func (t pruneTarget) describe() string { @@ -175,21 +176,19 @@ func (t pruneTarget) describe() string { // whole prune would block the instance creation the check is looking for. // Saving the record before GenerateBundle would close it, and that is a change // to the run path. -func pruneTargets(s *store.Store, all bool, now time.Time) ([]pruneTarget, error) { +func pruneTargets(s *store.Store, all bool) ([]pruneTarget, error) { staging, err := s.StagingDirs() if err != nil { return nil, err } var targets []pruneTarget for _, name := range staging { - dir := filepath.Join(s.RootDir(), "images", name) - bytes, touched := dirUsage(dir) - if pullInFlight(dir, now, touched) { - log.Debugf("leaving %s alone: a pull may still be using it", name) + if s.StagingInFlight(name) { + log.Debugf("leaving %s alone: a pull is using it", name) continue } reason := "leftover from an interrupted pull" - if displaced(dir) { + if store.Displaced(name) { // Not the same thing as a half-written `.tmp-`: this is the // previous image, renamed aside by a pull that then died before // deleting it. It is a whole image, and the line should say so @@ -200,7 +199,7 @@ func pruneTargets(s *store.Store, all bool, now time.Time) ([]pruneTarget, error name: name, staging: true, reason: reason, - bytes: bytes, + bytes: store.DirDiskUsage(filepath.Join(s.RootDir(), "images", name)), }) } @@ -255,7 +254,7 @@ func pruneTargets(s *store.Store, all bool, now time.Time) ([]pruneTarget, error t := pruneTarget{ name: name, reason: reason, - bytes: dirDiskUsage(s.ImageDir(name)), + bytes: store.DirDiskUsage(s.ImageDir(name)), } if recorded { t.ref = img.Ref @@ -323,127 +322,3 @@ func newer(a, b *store.ImageMetadata) bool { } return a.PulledAt.After(b.PulledAt) } - -// pullInFlight reports whether a staging directory may still belong to a -// running pull. See pullStagingGrace for why both halves are needed. -// -// The two staging kinds date differently. A `.tmp-` is written into for -// as long as the pull runs, so its age is the newest mtime anywhere in the -// tree: the directory's own mtime only moves when an entry is created directly -// in it, four times over a whole pull, and an image big enough to spend an -// hour unpacking would look untouched. -// -// A `.old-` is never written into. It arrives by rename at commit time -// and carries the displaced image's mtimes, which say when that image was -// unpacked and nothing about the pull holding it. Reading them let prune -// delete the rollback copy of a live pull whose previous image happened to be -// a month old. Its age is the directory's own ctime, which the rename sets. -func pullInFlight(dir string, now, touched time.Time) bool { - fi, err := os.Stat(dir) - if err != nil { - return false - } - if displaced(dir) { - touched = statusChangeTime(fi) - } - if now.Sub(touched) > pullStagingGrace { - return false - } - return processAlive(stagingPID(dir)) -} - -// displaced reports whether a staging name is the `.old-` kind. -func displaced(dir string) bool { - return strings.Contains(filepath.Base(dir), ".old-") -} - -// statusChangeTime is the inode's ctime, which a rename updates and a copy of -// the file times does not. Zero when the platform does not report one, which -// reads as ancient and prunes. -func statusChangeTime(fi os.FileInfo) time.Time { - st, ok := fi.Sys().(*syscall.Stat_t) - if !ok { - return time.Time{} - } - return time.Unix(st.Ctimespec.Sec, st.Ctimespec.Nsec) -} - -// stagingPID reads the pid out of a `.tmp-` name, or 0 when the -// name does not carry one. -func stagingPID(dir string) int { - name := filepath.Base(dir) - i := strings.LastIndex(name, "-") - if i < 0 { - return 0 - } - pid, err := strconv.Atoi(name[i+1:]) - if err != nil || pid <= 0 { - return 0 - } - return pid -} - -// processAlive reports whether a pid names a live process. EPERM counts: the -// process exists, it just belongs to somebody else. -func processAlive(pid int) bool { - if pid <= 0 { - return false - } - err := syscall.Kill(pid, 0) - return err == nil || errors.Is(err, syscall.EPERM) -} - -// dirDiskUsage sums what a directory tree actually occupies, so the figure -// printed is the space the host gets back rather than the size of the layers -// the image was pulled from. -// -// Blocks, not sizes: an unpacked rootfs is mostly small files, each rounded up -// to a block, and a sparse file occupies less than it claims. Hard links are -// counted once -- unpack creates them for repeated layer entries, and counting -// each name would overstate the total. -// -// APFS clones are counted in full, since each clone reports the blocks it -// shares. Nothing hull writes under images/ is a clone of another entry there, -// so the figure is exact for a store hull built and an upper bound for one -// somebody has copied inside with `cp -c`. -// -// Returns what it managed to measure. A walk that fails part-way gives a low -// figure in a report; refusing to delete over it would be worse. -func dirDiskUsage(dir string) int64 { - total, _ := dirUsage(dir) - return total -} - -// dirUsage is dirDiskUsage plus the newest mtime it saw, which is how the -// staging sweep tells a pull in flight from one that died. One walk answers -// both questions, and the staging path needs both. -func dirUsage(dir string) (int64, time.Time) { - var total int64 - var newest time.Time - seen := make(map[uint64]bool) - _ = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return nil - } - info, err := d.Info() - if err != nil { - return nil - } - st, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return nil - } - if mod := info.ModTime(); mod.After(newest) { - newest = mod - } - if st.Nlink > 1 { - if seen[st.Ino] { - return nil - } - seen[st.Ino] = true - } - total += st.Blocks * 512 - return nil - }) - return total, newest -} diff --git a/cmd/hull/prune_test.go b/cmd/hull/prune_test.go index 91d7bfa..b4bd523 100644 --- a/cmd/hull/prune_test.go +++ b/cmd/hull/prune_test.go @@ -23,6 +23,7 @@ import ( "sort" "strconv" "strings" + "syscall" "testing" "time" @@ -54,7 +55,7 @@ func TestPruneKeepsTheUsableImage(t *testing.T) { s := newCacheTestStore(t) seedImage(t, s, true) - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -100,7 +101,7 @@ func TestPruneKeepsBothPlatformsOfATag(t *testing.T) { Ref: cacheTestRef, Digest: otherTestDigest, Platform: "linux/amd64", }, true) - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -115,7 +116,7 @@ func TestPruneRemovesAnUnusableImage(t *testing.T) { s := newCacheTestStore(t) seedImage(t, s, false) - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -157,7 +158,7 @@ func TestPruneLeavesAnEntryThatIsNotADigest(t *testing.T) { t.Fatalf("MkdirAll: %v", err) } - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -175,7 +176,7 @@ func TestPruneAllRemovesAnUnusedImage(t *testing.T) { s := newCacheTestStore(t) seedImage(t, s, true) - targets, err := pruneTargets(s, true, time.Now()) + targets, err := pruneTargets(s, true) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -191,7 +192,7 @@ func TestPruneNeverRemovesAHeldImage(t *testing.T) { seedImage(t, s, false) // unusable, so it would go on every other ground seedInstance(t, s, "held", cacheTestDigest, "stopped", 0) - targets, err := pruneTargets(s, true, time.Now()) + targets, err := pruneTargets(s, true) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -204,15 +205,7 @@ func TestPruneSweepsStagingLeftovers(t *testing.T) { s := newCacheTestStore(t) seedImage(t, s, true) for _, suffix := range []string{".tmp-", ".old-"} { - dir := stagingDir(t, s, cacheTestDigest+suffix+"999999") - // Age the whole tree, not just the directory: a `.tmp-` is dated by - // the newest mtime under it, so ageing the top alone changes nothing. - old := time.Now().Add(-2 * pullStagingGrace) - for _, p := range []string{filepath.Join(dir, "rootfs"), dir} { - if err := os.Chtimes(p, old, old); err != nil { - t.Fatalf("Chtimes: %v", err) - } - } + stagingDir(t, s, cacheTestDigest+suffix+"999999") } var out bytes.Buffer @@ -232,12 +225,22 @@ func TestPruneSweepsStagingLeftovers(t *testing.T) { } // A staging directory belonging to a pull in flight must survive: prune has to -// be safe to run at any moment, and deleting it fails that pull. +// be safe to run at any moment, and deleting it fails that pull. The pull +// holds a lock on it, and that lock is the whole of the evidence: no pid, no +// age, however old the tree looks. func TestPruneLeavesAPullInFlightAlone(t *testing.T) { s := newCacheTestStore(t) - dir := stagingDir(t, s, cacheTestDigest+".tmp-"+strconv.Itoa(os.Getpid())) + name := cacheTestDigest + ".tmp-" + strconv.Itoa(os.Getpid()) + dir := stagingDir(t, s, name) + old := time.Now().Add(-48 * time.Hour) + for _, p := range []string{filepath.Join(dir, "rootfs"), dir} { + if err := os.Chtimes(p, old, old); err != nil { + t.Fatalf("Chtimes: %v", err) + } + } + held := holdStagingDir(t, dir) - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -245,33 +248,60 @@ func TestPruneLeavesAPullInFlightAlone(t *testing.T) { t.Fatalf("prune would remove a pull in flight: %v", targetNames(targets)) } - // The freshness that protects it is the newest mtime in the tree, not the - // staging directory's own. A pull writes into rootfs/, and an image big - // enough can spend an hour there without touching the directory above. - old := time.Now().Add(-2 * pullStagingGrace) - if err := os.Chtimes(dir, old, old); err != nil { - t.Fatalf("Chtimes: %v", err) + // And a removal decided before the pull started is refused, not + // carried out: the lock is taken again at the delete. + var out bytes.Buffer + if err := pruneStoreIn(&out, s, false, false, ""); err != nil { + t.Fatalf("pruneStoreIn: %v", err) } - targets, err = pruneTargets(s, false, time.Now()) + if _, err := os.Stat(dir); err != nil { + t.Fatalf("prune removed a locked staging directory: %v", err) + } + + _ = held.Close() + targets, err = pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } - if len(targets) != 0 { - t.Fatalf("a pull still writing into rootfs/ was pruned: %v", targetNames(targets)) + if len(targets) != 1 { + t.Fatalf("a released staging directory was still protected: %v", targetNames(targets)) } +} - // Age the whole tree, and the pid alone must not protect it: pid numbers - // are recycled, and no pull takes an hour. - if err := os.Chtimes(filepath.Join(dir, "rootfs"), old, old); err != nil { - t.Fatalf("Chtimes: %v", err) +// A removal the listing approved is refused when a pull has taken the +// directory since, and the run says so instead of failing. +func TestPruneReportsAStagingDirectoryTakenSinceTheListing(t *testing.T) { + s := newCacheTestStore(t) + name := cacheTestDigest + ".tmp-" + strconv.Itoa(os.Getpid()) + dir := stagingDir(t, s, name) + holdStagingDir(t, dir) + + var out bytes.Buffer + targets := []pruneTarget{{name: name, staging: true, reason: "leftover from an interrupted pull"}} + if _, err := removePruneTargets(&out, s, targets, false); err != nil { + t.Fatalf("removing a target a pull took since: %v", err) } - targets, err = pruneTargets(s, false, time.Now()) + if !strings.Contains(out.String(), "Left "+name+" alone") { + t.Errorf("the run does not say the directory was left alone: %q", out.String()) + } + if _, err := os.Stat(dir); err != nil { + t.Errorf("the locked directory was removed: %v", err) + } +} + +// holdStagingDir takes the lock a pull holds on its staging directory, and +// releases it when the test ends. +func holdStagingDir(t *testing.T, dir string) *os.File { + t.Helper() + f, err := os.Open(dir) if err != nil { - t.Fatalf("pruneTargets: %v", err) + t.Fatalf("Open: %v", err) } - if len(targets) != 1 { - t.Fatalf("an hour-old staging directory was still protected: %v", targetNames(targets)) + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + t.Fatalf("Flock: %v", err) } + t.Cleanup(func() { _ = f.Close() }) + return f } func TestPruneDryRunRemovesNothing(t *testing.T) { @@ -307,44 +337,6 @@ func TestPruneEmptyStore(t *testing.T) { } } -// The figure printed is the space the host gets back, so it counts blocks and -// counts a hard-linked file once. Unpack creates those for repeated entries. -func TestDirDiskUsageCountsAHardLinkOnce(t *testing.T) { - dir := t.TempDir() - body := bytes.Repeat([]byte("x"), 64*1024) - first := filepath.Join(dir, "a") - if err := os.WriteFile(first, body, 0o600); err != nil { - t.Fatalf("WriteFile: %v", err) - } - single := dirDiskUsage(dir) - if single < int64(len(body)) { - t.Fatalf("dirDiskUsage = %d for a %d-byte file", single, len(body)) - } - if err := os.Link(first, filepath.Join(dir, "b")); err != nil { - t.Fatalf("Link: %v", err) - } - if got := dirDiskUsage(dir); got != single { - t.Errorf("a hard link changed the total from %d to %d", single, got) - } -} - -func TestStagingPID(t *testing.T) { - for _, tc := range []struct { - name string - want int - }{ - {cacheTestDigest + ".tmp-4242", 4242}, - {cacheTestDigest + ".old-1", 1}, - {cacheTestDigest + ".tmp-", 0}, - {cacheTestDigest + ".tmp-abc", 0}, - {cacheTestDigest, 0}, - } { - if got := stagingPID(filepath.Join("/store/images", tc.name)); got != tc.want { - t.Errorf("stagingPID(%q) = %d, want %d", tc.name, got, tc.want) - } - } -} - // The blocking case from review. `run` picks the newest COMPLETE entry, so an // incomplete newer one must not supersede the complete older one: prune used // to remove the incomplete entry as unusable and the complete one as @@ -371,8 +363,9 @@ func TestPruneKeepsTheLastCompleteEntryOfATag(t *testing.T) { } // A `.old-` carries the displaced image's mtimes, which say when that -// image was unpacked. Reading them as the entry's age let prune delete the -// rollback copy of a pull that is still running. +// image was unpacked and nothing about the pull holding it; prune once read +// them as the entry's age and deleted the rollback copy of a running pull. +// The pull's lock on it is the evidence now, whatever the tree's dates say. func TestPruneKeepsAFreshlyDisplacedImage(t *testing.T) { s := newCacheTestStore(t) dir := stagingDir(t, s, cacheTestDigest+".old-"+strconv.Itoa(os.Getpid())) @@ -382,8 +375,9 @@ func TestPruneKeepsAFreshlyDisplacedImage(t *testing.T) { t.Fatalf("Chtimes: %v", err) } } + holdStagingDir(t, dir) - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -398,7 +392,7 @@ func TestPruneNamesWhatEachStagingKindIs(t *testing.T) { stagingDir(t, s, cacheTestDigest+".tmp-"+strconv.Itoa(stalePID)) stagingDir(t, s, cacheTestDigest+".old-"+strconv.Itoa(stalePID)) - targets, err := pruneTargets(s, false, time.Now()) + targets, err := pruneTargets(s, false) if err != nil { t.Fatalf("pruneTargets: %v", err) } @@ -417,3 +411,22 @@ func TestPruneNamesWhatEachStagingKindIs(t *testing.T) { t.Errorf(".old- reason = %q", r) } } + +// A leftover prune listed and a pull's sweep took first is reported as +// already gone, and its bytes are not claimed as reclaimed by this run. +func TestPruneDoesNotClaimWhatAnotherSweeperFreed(t *testing.T) { + s := newCacheTestStore(t) + name := cacheTestDigest + ".tmp-" + strconv.Itoa(stalePID) + var out bytes.Buffer + targets := []pruneTarget{{name: name, staging: true, reason: "leftover from an interrupted pull", bytes: 4096}} + reclaimed, err := removePruneTargets(&out, s, targets, false) + if err != nil { + t.Fatalf("removing a target that is already gone: %v", err) + } + if reclaimed != 0 { + t.Errorf("reclaimed %d bytes nothing in this run freed", reclaimed) + } + if !strings.Contains(out.String(), "Already gone: "+name) { + t.Errorf("the run does not say the directory was already gone: %q", out.String()) + } +} diff --git a/cmd/hull/pull.go b/cmd/hull/pull.go index 0855309..9ed1283 100644 --- a/cmd/hull/pull.go +++ b/cmd/hull/pull.go @@ -56,6 +56,7 @@ func pullImage(ctx context.Context, cmd *cli.Command) error { } client := ociclient.New(s) + client.InstanceLive = instanceMayHaveVMM fmt.Printf("Pulling %s...\n", imageRef) result, err := client.PullPlatform(ctx, imageRef, cmd.String("platform")) diff --git a/cmd/hull/run.go b/cmd/hull/run.go index e261a55..110d94f 100644 --- a/cmd/hull/run.go +++ b/cmd/hull/run.go @@ -274,6 +274,7 @@ func runInstance(ctx context.Context, cmd *cli.Command) error { // Create OCI client client := ociclient.New(s) + client.InstanceLive = instanceMayHaveVMM log.Debugf("Running instance %s from image %s", instanceName, imageRef) diff --git a/docs/storage.md b/docs/storage.md index 40754bf..8f72cba 100644 --- a/docs/storage.md +++ b/docs/storage.md @@ -142,8 +142,8 @@ Without `--all`, prune removes only what no run could ever choose: | Removed | Why it is dead | |---|---| -| `.tmp-` | a half-unpacked image from an interrupted pull. Otherwise swept only at the start of the next pull | -| `.old-` | the previous image, renamed aside by a pull that died before deleting it | +| `.tmp-` | a half-unpacked image from an interrupted pull. Otherwise swept only at the start of the next pull, which applies the same lock check | +| `.old--` | the previous image, renamed aside by an interrupted pull or image removal | | an image with no `rootfs/`, or one stamped with an older unpack schema | it satisfies no cache lookup; the next run re-pulls it | | an image directory whose `image.json` cannot be read | invisible to `hull images`, and to everything else | | a digest superseded by a later **complete** pull of the same reference and platform | `hull run` already resolves that group to the newest complete entry. A run that pins the older digest re-pulls it | @@ -152,25 +152,22 @@ With `--all` it also removes every image no instance refers to, which is the whole cache on a machine with nothing running. Nothing prune removes is unrecoverable: every image can be pulled again. -An image any instance refers to is never pruned, running or stopped. Use -`hull rmi --force` for that case. Prune does not touch instances, boot assets -or named volumes. +An image any instance refers to is never pruned, running or stopped. Stop +running instances first, then use `hull rmi --force` for that case. Prune does +not touch instances, boot assets or named volumes. Only a complete image supersedes another. An incomplete newer entry is removed on its own account and never takes the last usable entry of a tag with it. -A staging directory is left alone while it may still belong to a pull in -flight: its pid names a live process **and** it is younger than an hour. The -pid alone is not enough, because pid numbers are recycled and the entry would -then be immortal. - -The two staging kinds are dated differently. A `.tmp-` is written into for -as long as the pull runs, so its age is the newest mtime anywhere in the tree; -the directory's own mtime moves about four times over a whole pull and would -make a long one look abandoned. A `.old-` is never written into: it -arrives by rename and carries the displaced image's mtimes, which say when that -image was unpacked, so its age is the directory's own ctime, which the rename -sets. +A staging directory is left alone while a pull is using it. The pull holds a +lock on the directory itself (a `flock` on its descriptor) for as long as it +unpacks into it, and on the previous image from the moment it is renamed aside +until it is deleted or put back. The kernel drops the lock when the process +exits, however it exits. A sweeper tries the lock without waiting: busy means +in flight, and it deletes while holding it, so two sweepers cannot race. The +sweep a pull runs before unpacking applies the same rule, so two pulls into one +store at the same time leave each other's staging directories alone. The pid +in the name only keeps two pulls' directories apart. ## Layout @@ -184,7 +181,8 @@ sets. │ ├── image.json │ ├── oci-config.json │ ├── rootfs/ the unpacked image -│ └── (unpack-schema stamp) +│ ├── unpack-schema the layout version rootfs/ was written with +│ └── rootfs-entries how many entries rootfs/ held when published ├── assets/ generic boot assets, shared across instances │ ├── Image the kernel; bzImage on amd64 │ ├── container-initrd @@ -451,15 +449,39 @@ stamp are all present. Metadata alone is a miss, so an interrupted pull heals itself on the next run instead of poisoning every later one. The current layout version is 3; bumping it makes every older rootfs a miss that gets re-unpacked. +A pull also records how many entries `rootfs/` held when it was published, in +`rootfs-entries`. A pull that resolves to a digest the store already holds +counts them again before deciding there is nothing to unpack, and re-unpacks +the image when entries have gone missing since: an image that lost part of +its tree after publication is repaired by pulling it again, rather than only +by `hull rmi`. The count is taken after the last layer is unpacked, so it +cannot tell a tree that was already short when it was published; that case +still needs `hull rmi`. An image an instance refers to is never displaced +this way, running or stopped: a running guest has the rootfs shared in, and a +stopped instance expects it back on start. The pull warns, names the +instances, and serves the image as is. Stop the instances and use +`hull rmi --force`, or remove them with `hull rm --force`, then pull again. +The pull checks holders again before publishing, but a run only becomes +visible to this check once `launchVMM` records its image digest, after bundle +preparation and rootfs cloning. A holder recorded between the check and the +swap is also outside this protection. An image pulled by a hull from before the +count existed carries none and is trusted. The count is only read on the pull +path, so `hull run` pays for it only under `--pull=always`; with the default +policy an image it already holds is served as is. + A pull commits by directory rename, not by deleting in place. Layers unpack into -`.tmp-`, any existing image is displaced to `.old-`, +`.tmp-`, any existing image is displaced to `.old--`, and the slow recursive delete happens only after the new image is published. Leftover staging directories are swept at the start of the next pull and by -`hull prune`, never satisfy a cache lookup, and never appear in `hull images`. +`hull prune`, once the pull that made them no longer holds its lock on them; +they never satisfy a cache lookup, and never appear in `hull images`. `hull rmi` and `hull prune` remove an image the same way, renaming the directory aside before deleting it, so an interrupted removal leaves a staging -name rather than an image with half a rootfs. +name rather than an image with half a rootfs. Each displacement has a unique +suffix so concurrent commits or removals in one process do not reuse a name. +The displaced directory stays locked through deletion, so prune skips an +active removal. The store lock is released before the recursive delete. Pulling a republished tag adds a new digest without retiring the old one, so several image directories can answer one tag. The most recently pulled complete diff --git a/pkg/ociclient/client.go b/pkg/ociclient/client.go index 8dd42d9..ebe4370 100644 --- a/pkg/ociclient/client.go +++ b/pkg/ociclient/client.go @@ -38,6 +38,13 @@ type Client struct { store *store.Store // Quiet suppresses pull progress output on stderr. Quiet bool + // InstanceLive says whether an instance record can still have a VMM + // behind it, which is what keeps a pull from displacing the image that + // VMM is running on. hull wires in the check rmi uses, which looks at + // the process; nil falls back to the record's status alone, which reads + // a record left at "running" by a death as live and so keeps the image + // for it until something reconciles the record. + InstanceLive func(*store.InstanceState) bool } // PullResult contains the result of a successful image pull @@ -182,17 +189,45 @@ func (c *Client) PullPlatform(ctx context.Context, ref, platformStr string) (*Pu // This is what makes --pull=always affordable: it costs a manifest lookup // rather than a full re-download and re-unpack of an unchanged image. // The metadata is rewritten so PulledAt and Ref track this resolution. + // + // Complete is not the same as whole. An image can lose part of its tree + // after it was published and look no different from here. The entry + // count the pull recorded is checked, and fewer entries than recorded + // falls through to a fresh unpack, which displaces the damaged image the + // way any re-pull would. + // + // Unless an instance refers to it. A running one has the rootfs shared + // into its guest, and displacing the directory would pull the root + // filesystem out from under it; rmi and prune refuse the same way. The + // damaged image is served as is, with the way out in the warning. if c.store.ImageComplete(digestStr) { - log.Debugf("image %s already present, skipping unpack", digestStr) - if _, err := c.store.SaveImage(digestStr, metadata); err != nil { - return nil, fmt.Errorf("failed to refresh image metadata: %w", err) + damaged := c.store.VerifyImage(digestStr) + if damaged != nil { + holders, err := c.imageHeld(digestStr) + if err != nil { + return nil, err + } + if len(holders) > 0 { + log.Warnf("image %s is damaged (%v) but instances %s refer to it, so it is kept as is; "+ + "stop them and use `hull rmi --force`, or remove them with `hull rm --force`, then pull again", + digestStr, damaged, strings.Join(holders, ", ")) + damaged = nil + } else { + log.Warnf("image %s will be unpacked again: %v", digestStr, damaged) + } + } + if damaged == nil { + log.Debugf("image %s already present, skipping unpack", digestStr) + if _, err := c.store.SaveImage(digestStr, metadata); err != nil { + return nil, fmt.Errorf("failed to refresh image metadata: %w", err) + } + return &PullResult{ + Digest: digestStr, + Size: totalSize, + Labels: labels, + Platforms: []string{fmt.Sprintf("%s/%s", config.OS, config.Architecture)}, + }, nil } - return &PullResult{ - Digest: digestStr, - Size: totalSize, - Labels: labels, - Platforms: []string{fmt.Sprintf("%s/%s", config.OS, config.Architecture)}, - }, nil } // Build the image atomically: unpack into a temp sibling and rename it @@ -204,15 +239,37 @@ func (c *Client) PullPlatform(ctx context.Context, ref, platformStr string) (*Pu if err := os.MkdirAll(imagesRoot, 0700); err != nil { return nil, fmt.Errorf("failed to create images dir: %w", err) } - for _, pattern := range []string{"*.tmp-*", "*.old-*"} { - if stale, err := filepath.Glob(filepath.Join(imagesRoot, pattern)); err == nil { - for _, d := range stale { - _ = os.RemoveAll(d) - } - } + // Sweep what interrupted pulls left behind, and only that: another pull + // may be unpacking into its own staging directory right now, or holding + // the previous image aside while it commits, and holds a lock on it + // either way. The sweep is best effort; a leftover it could not remove + // is prune's to report. + removed, sweepErr := c.store.SweepStaging() + if len(removed) > 0 { + log.Debugf("swept %d leftover(s) of interrupted pulls: %v", len(removed), removed) } + if sweepErr != nil { + log.Debugf("could not sweep every pull leftover: %v", sweepErr) + } + // The staging directory is locked for as long as this pull uses it, so + // a sweep in another process leaves it alone; see store.CreateStaging. + // The deferred delete runs before the deferred release, so an abandoned + // pull takes its directory with it while still holding the lock. Once + // the commit has published the directory the name is no longer this + // pull's: a second pull of the same digest in this process can have + // made a fresh one under it, and the delete must not take that. tmpDir := filepath.Join(imagesRoot, fmt.Sprintf("%s.tmp-%d", digestStr, os.Getpid())) - defer func() { _ = os.RemoveAll(tmpDir) }() + staging, err := store.CreateStaging(tmpDir) + if err != nil { + return nil, err + } + published := false + defer func() { _ = staging.Close() }() + defer func() { + if !published { + _ = os.RemoveAll(tmpDir) + } + }() prog := newProgress(c.Quiet, len(layers), totalSize) if err := UnpackLayers(layers, filepath.Join(tmpDir, "rootfs"), prog); err != nil { @@ -234,31 +291,39 @@ func (c *Client) PullPlatform(ctx context.Context, ref, platformStr string) (*Pu if err := store.WriteUnpackSchema(tmpDir); err != nil { return nil, fmt.Errorf("failed to stamp the unpack schema: %w", err) } + // And the size of the tree it was published with, so a later pull can tell + // whether it is still all there. + if err := store.WriteRootfsEntries(tmpDir); err != nil { + return nil, fmt.Errorf("failed to record the rootfs entry count: %w", err) + } - // Commit by swapping directories, never by deleting in place. RemoveAll - // on a populated rootfs takes seconds and deletes in readdir order, so an - // interrupt during it could strip the rootfs while leaving image.json -- - // exactly the half-state that made every later run fail. Renaming the old - // directory aside is atomic; the slow delete then happens once the new - // image is already published, where an interrupt is harmless (the leftover - // is swept by the *.old-* glob above). - finalDir := filepath.Join(imagesRoot, digestStr) - oldDir := filepath.Join(imagesRoot, fmt.Sprintf("%s.old-%d", digestStr, os.Getpid())) - swapped := false - if _, err := os.Stat(finalDir); err == nil { - if err := os.Rename(finalDir, oldDir); err != nil { - return nil, fmt.Errorf("failed to displace the previous image: %w", err) - } - swapped = true + // An instance may have started on the image while the layers were + // downloading, so check the holder records again before committing. A run + // is only visible here once launchVMM records its image digest; earlier + // bundle preparation and rootfs cloning are not covered, nor is a holder + // recorded between this check and the swap. Keep the image a recorded + // holder has and discard this pull's copy with the staging directory. + holders, err := c.imageHeld(digestStr) + if err != nil { + return nil, err } - if err := os.Rename(tmpDir, finalDir); err != nil { - if swapped { - _ = os.Rename(oldDir, finalDir) // put the usable image back + if len(holders) > 0 { + log.Warnf("image %s is in use by instances %s, so the image they have is kept as is; "+ + "stop them and use `hull rmi --force`, or remove them with `hull rm --force`, then pull again", + digestStr, strings.Join(holders, ", ")) + // The image on disk is theirs; the metadata only records what this + // pull resolved, as the early skip above records it. + if _, err := c.store.SaveImage(digestStr, metadata); err != nil { + return nil, fmt.Errorf("failed to refresh image metadata: %w", err) } - return nil, fmt.Errorf("failed to commit image: %w", err) - } - if swapped { - _ = os.RemoveAll(oldDir) + } else { + // Publish by swapping directories, under the store lock; see + // CommitImage for why neither half is optional, and why it takes + // the staging lock to release it itself. + if err := c.store.CommitImage(digestStr, tmpDir, staging); err != nil { + return nil, err + } + published = true } // Get available platforms @@ -272,6 +337,41 @@ func (c *Client) PullPlatform(ctx context.Context, ref, platformStr string) (*Pu }, nil } +// imageHeld returns the ids of the instances the image on disk must be kept +// for, which is every live holder, and the stopped ones too when the image +// is complete. +// +// A live instance has the image's rootfs shared into its guest, whatever +// the cache thinks of that image: a rootfs stamped with an older unpack +// schema is a miss here and a perfectly good root filesystem to a guest +// already booted on it, and replacing the directory under that guest is +// not a repair. rmi refuses over the same records, with the same test of +// liveness when the caller wired one in. A stopped instance only expects +// the image back on its next start, so a complete one is kept for it and +// an incomplete one is what this pull replaces for it. +func (c *Client) imageHeld(digest string) ([]string, error) { + holders, err := c.store.ImageHolders(digest) + if err != nil { + return nil, fmt.Errorf("failed to list the instances using image %s: %w", digest, err) + } + complete := c.store.ImageComplete(digest) + var ids []string + for _, h := range holders { + if c.instanceLive(h) || complete { + ids = append(ids, h.ID) + } + } + return ids, nil +} + +// instanceLive applies InstanceLive, or the status-only fallback. +func (c *Client) instanceLive(inst *store.InstanceState) bool { + if c.InstanceLive != nil { + return c.InstanceLive(inst) + } + return inst.Status == "running" || inst.Status == store.StatusStarting +} + // keepIndexDigest carries an index digest already recorded for this image over // into metadata that has none. // diff --git a/pkg/ociclient/client_test.go b/pkg/ociclient/client_test.go index 164ec4a..ef05b28 100644 --- a/pkg/ociclient/client_test.go +++ b/pkg/ociclient/client_test.go @@ -20,11 +20,17 @@ import ( "compress/gzip" "context" "io" + "net/http" "net/http/httptest" "net/url" "os" "path/filepath" + "strconv" + "strings" + "sync/atomic" + "syscall" "testing" + "time" "github.com/brig-sh/hull/pkg/store" "github.com/google/go-containerregistry/pkg/name" @@ -75,8 +81,32 @@ func testLayer(t *testing.T, content string) v1.Layer { // returns its reference. func testRegistry(t *testing.T, content string) string { t.Helper() - srv := httptest.NewServer(registry.New()) + return testRegistryWithBlobHook(t, content, nil) +} + +// testRegistryWithBlobHook is testRegistry with onBlob called before the +// layer blob a pull fetches, once the image is in place: a way to act in the +// middle of a pull, between its decision to download and its commit. The +// layer only, not the config blob: the pull reads the config before it +// decides anything, so a hook on that fires before the decision. +func testRegistryWithBlobHook(t *testing.T, content string, onBlob func()) string { + t.Helper() + layer := testLayer(t, content) + layerDigest, err := layer.Digest() + if err != nil { + t.Fatalf("layer digest: %v", err) + } + backend := registry.New() + var serving atomic.Bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if onBlob != nil && serving.Load() && r.Method == http.MethodGet && + strings.HasSuffix(r.URL.Path, "/blobs/"+layerDigest.String()) { + onBlob() + } + backend.ServeHTTP(w, r) + })) t.Cleanup(srv.Close) + defer serving.Store(true) u, err := url.Parse(srv.URL) if err != nil { @@ -86,7 +116,7 @@ func testRegistry(t *testing.T, content string) string { if err != nil { t.Fatalf("ParseReference: %v", err) } - img, err := mutate.AppendLayers(empty.Image, testLayer(t, content)) + img, err := mutate.AppendLayers(empty.Image, layer) if err != nil { t.Fatalf("AppendLayers: %v", err) } @@ -287,3 +317,330 @@ func TestPullHonorsContextCancellation(t *testing.T) { t.Fatal("a cancelled context must stop the pull") } } + +// Two pulls into one store at the same time (#114). The sweep a pull runs +// before unpacking must leave the other pull's staging directories alone: the +// `.tmp-` it is unpacking into, and the `.old-` it is holding the +// previous image in until its commit is through. Removing the first truncates +// the other pull's image; removing the second leaves its rollback nothing to +// restore. The other pull holds a lock on each, which is what the sweep +// reads; the leftovers of a pull that is gone, whose locks died with it, are +// still swept. +func TestPullLeavesAConcurrentPullsStagingAlone(t *testing.T) { + ref := testRegistry(t, "hello from the test layer") + c, s := newClient(t) + + const other = "sha256:c408baae42f5c74c0661fbc20a289fd23d4322988e52c88cd54108e5c4c74893" + plant := func(name string, held bool) string { + dir := filepath.Join(s.RootDir(), "images", name) + if err := os.MkdirAll(filepath.Join(dir, "rootfs", "usr"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if held { + f, err := os.Open(dir) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + t.Fatalf("Flock: %v", err) + } + t.Cleanup(func() { _ = f.Close() }) + } + return dir + } + inFlight := plant(other+".tmp-1", true) + rollback := plant(other+".old-1", true) + gone := plant(other+".tmp-2", false) + + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("Pull: %v", err) + } + for _, dir := range []string{inFlight, rollback} { + if _, err := os.Stat(filepath.Join(dir, "rootfs", "usr")); err != nil { + t.Errorf("the pull swept a concurrent pull's %s: %v", filepath.Base(dir), err) + } + } + if _, err := os.Stat(gone); !os.IsNotExist(err) { + t.Errorf("the leftover of a dead pull survived: %v", err) + } +} + +// A legacy `.old-` whose best-effort delete did not finish is still +// swept before a new pull unpacks, even when its pid is this process's own. +// Its directory lock, not the pid in its name, decides whether it is busy. +func TestPullCommitsOverItsOwnOldLeftover(t *testing.T) { + ref := testRegistry(t, "hello from the test layer") + c, s := newClient(t) + + first, err := c.Pull(context.Background(), ref) + if err != nil { + t.Fatalf("first Pull: %v", err) + } + // What a commit whose RemoveAll did not finish leaves behind, with the + // image incomplete so the second pull has to commit again. + leftover := filepath.Join(s.RootDir(), "images", first.Digest+".old-"+strconv.Itoa(os.Getpid())) + if err := os.MkdirAll(filepath.Join(leftover, "rootfs"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Join(leftover, "rootfs", "stale"), []byte("x"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.Remove(filepath.Join(s.RootDir(), "images", first.Digest, "unpack-schema")); err != nil { + t.Fatalf("Remove: %v", err) + } + + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("second Pull over an own .old- leftover: %v", err) + } + if !s.ImageComplete(first.Digest) { + t.Error("the second pull did not publish a complete image") + } + if _, err := os.Stat(leftover); !os.IsNotExist(err) { + t.Errorf("the own .old- leftover survived the commit: %v", err) + } +} + +// A store left with a truncated image by the race in #114 looks complete, and +// before the count was recorded nothing but `hull rmi` got it out of that +// state. Pulling the same reference again must notice the missing entry and +// unpack the image afresh. +func TestPullRepairsATruncatedImage(t *testing.T) { + ref := testRegistry(t, "hello from the test layer") + c, s := newClient(t) + + first, err := c.Pull(context.Background(), ref) + if err != nil { + t.Fatalf("first Pull: %v", err) + } + lost := filepath.Join(s.RootDir(), "images", first.Digest, "rootfs", testFileName) + if err := os.Remove(lost); err != nil { + t.Fatalf("Remove: %v", err) + } + if !s.ImageComplete(first.Digest) { + t.Fatal("precondition: the truncated image should still look complete") + } + + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("second Pull: %v", err) + } + if _, err := os.Stat(lost); err != nil { + t.Errorf("the second pull reused the truncated image: %v", err) + } + if err := s.VerifyImage(first.Digest); err != nil { + t.Errorf("the repaired image fails verification: %v", err) + } + entries, err := os.ReadDir(filepath.Join(s.RootDir(), "images")) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + for _, e := range entries { + if e.Name() != first.Digest { + t.Errorf("the repair left %s behind", e.Name()) + } + } +} + +// A damaged image an instance refers to is not displaced: a running guest has +// that rootfs shared in, and a stopped instance expects it back on start. The +// pull warns and serves what is there, as rmi and prune refuse the same way. +func TestPullKeepsADamagedImageAnInstanceHolds(t *testing.T) { + ref := testRegistry(t, "hello from the test layer") + c, s := newClient(t) + + first, err := c.Pull(context.Background(), ref) + if err != nil { + t.Fatalf("first Pull: %v", err) + } + lost := filepath.Join(s.RootDir(), "images", first.Digest, "rootfs", testFileName) + if err := os.Remove(lost); err != nil { + t.Fatalf("Remove: %v", err) + } + if _, err := s.CreateInstance("holder"); err != nil { + t.Fatalf("CreateInstance: %v", err) + } + if err := s.SaveInstance(&store.InstanceState{ID: "holder", ImageDigest: first.Digest, Status: "running"}); err != nil { + t.Fatalf("SaveInstance: %v", err) + } + sentinel := filepath.Join(s.RootDir(), "images", first.Digest, ".sentinel") + if err := os.WriteFile(sentinel, []byte("held"), 0600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("second Pull: %v", err) + } + if _, err := os.Stat(sentinel); err != nil { + t.Error("the pull displaced an image an instance refers to") + } + if _, err := os.Stat(lost); err == nil { + t.Error("the pull re-unpacked an image an instance refers to") + } +} + +// The check before the download is not the last word: the download takes +// minutes on a real image, and an instance can start on the cached image in +// that time, with its rootfs shared straight into the guest. The commit has +// to look again, and keep the image the instance has. +func TestPullKeepsAnImageAnInstanceStartedOnDuringThePull(t *testing.T) { + c, s := newClient(t) + // The hook runs on the server's goroutine; the digest it needs is only + // known after the first pull, so it is handed over atomically. + var digest atomic.Pointer[string] + var started atomic.Bool + ref := testRegistryWithBlobHook(t, "hello from the test layer", func() { + d := digest.Load() + if d == nil || started.Swap(true) { + return + } + if _, err := s.CreateInstance("late"); err != nil { + t.Errorf("CreateInstance: %v", err) + } + if err := s.SaveInstance(&store.InstanceState{ID: "late", ImageDigest: *d, Status: "running"}); err != nil { + t.Errorf("SaveInstance: %v", err) + } + }) + + first, err := c.Pull(context.Background(), ref) + if err != nil { + t.Fatalf("first Pull: %v", err) + } + digest.Store(&first.Digest) + lost := filepath.Join(s.RootDir(), "images", first.Digest, "rootfs", testFileName) + if err := os.Remove(lost); err != nil { + t.Fatalf("Remove: %v", err) + } + sentinel := filepath.Join(s.RootDir(), "images", first.Digest, ".sentinel") + if err := os.WriteFile(sentinel, []byte("held"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + before := time.Now() + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("second Pull: %v", err) + } + if !started.Load() { + t.Fatal("precondition: the instance was never started during the pull") + } + if _, err := os.Stat(sentinel); err != nil { + t.Error("the pull displaced an image an instance started on during the download") + } + // Kept as is, but still resolved: the record says this pull happened, + // as it does when the early skip keeps an image. + img, err := s.GetImage(first.Digest) + if err != nil { + t.Fatalf("GetImage: %v", err) + } + if img.PulledAt.Before(before) { + t.Errorf("the kept image's record was not refreshed: pulled at %v, pull started %v", img.PulledAt, before) + } + staging, err := s.StagingDirs() + if err != nil { + t.Fatalf("StagingDirs: %v", err) + } + if len(staging) != 0 { + t.Errorf("the discarded pull left %v behind", staging) + } +} + +// The cache can call an image incomplete while a guest is running on it: a +// hull with a bumped unpack schema reads the old rootfs as a miss, and the +// instance booted before the upgrade has that very directory shared in. +// The pull must not displace it; a stopped instance on an incomplete image +// is the opposite case, waiting for exactly this pull. +func TestPullKeepsAnIncompleteImageALiveInstanceHolds(t *testing.T) { + for _, tc := range []struct { + status string + kept bool + }{ + {"running", true}, + {"stopped", false}, + } { + t.Run(tc.status, func(t *testing.T) { + ref := testRegistry(t, "hello from the test layer") + c, s := newClient(t) + first, err := c.Pull(context.Background(), ref) + if err != nil { + t.Fatalf("first Pull: %v", err) + } + imageDir := filepath.Join(s.RootDir(), "images", first.Digest) + // What a schema bump looks like from the store's side. + if err := os.WriteFile(filepath.Join(imageDir, "unpack-schema"), []byte("1\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if s.ImageComplete(first.Digest) { + t.Fatal("precondition: the image should read as incomplete") + } + if _, err := s.CreateInstance("holder"); err != nil { + t.Fatalf("CreateInstance: %v", err) + } + if err := s.SaveInstance(&store.InstanceState{ID: "holder", ImageDigest: first.Digest, Status: tc.status}); err != nil { + t.Fatalf("SaveInstance: %v", err) + } + sentinel := filepath.Join(imageDir, ".sentinel") + if err := os.WriteFile(sentinel, []byte("held"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("second Pull: %v", err) + } + _, err = os.Stat(sentinel) + if tc.kept && err != nil { + t.Error("the pull displaced an incomplete image a live instance is running on") + } + if !tc.kept && err == nil { + t.Error("the pull kept an incomplete image only a stopped instance refers to") + } + }) + } +} + +// The store cannot tell a VMM from a recycled pid, so hull wires in the +// check rmi uses. A record left at "running" by a death must not pin an +// incomplete image forever: with the caller's check saying it is dead, the +// pull replaces the image; without one, the status alone keeps it. +func TestPullAsksTheCallerWhetherAHolderIsLive(t *testing.T) { + for _, tc := range []struct { + name string + live func(*store.InstanceState) bool + kept bool + }{ + {"caller says dead", func(*store.InstanceState) bool { return false }, false}, + {"caller says live", func(*store.InstanceState) bool { return true }, true}, + {"no caller check", nil, true}, + } { + t.Run(tc.name, func(t *testing.T) { + ref := testRegistry(t, "hello from the test layer") + c, s := newClient(t) + c.InstanceLive = tc.live + first, err := c.Pull(context.Background(), ref) + if err != nil { + t.Fatalf("first Pull: %v", err) + } + imageDir := filepath.Join(s.RootDir(), "images", first.Digest) + if err := os.WriteFile(filepath.Join(imageDir, "unpack-schema"), []byte("1\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, err := s.CreateInstance("stale"); err != nil { + t.Fatalf("CreateInstance: %v", err) + } + if err := s.SaveInstance(&store.InstanceState{ID: "stale", ImageDigest: first.Digest, Status: "running", PID: 999999}); err != nil { + t.Fatalf("SaveInstance: %v", err) + } + sentinel := filepath.Join(imageDir, ".sentinel") + if err := os.WriteFile(sentinel, []byte("held"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if _, err := c.Pull(context.Background(), ref); err != nil { + t.Fatalf("second Pull: %v", err) + } + _, err = os.Stat(sentinel) + if tc.kept && err != nil { + t.Error("the pull displaced an image a holder the caller calls live is on") + } + if !tc.kept && err == nil { + t.Error("the pull kept an incomplete image for a record the caller calls dead") + } + }) + } +} diff --git a/pkg/store/image.go b/pkg/store/image.go index 403a304..ec80ca1 100644 --- a/pkg/store/image.go +++ b/pkg/store/image.go @@ -15,8 +15,10 @@ package store import ( + "crypto/rand" "errors" "fmt" + "io/fs" "os" "path/filepath" "regexp" @@ -59,44 +61,144 @@ func (s *Store) ImageDir(digest string) string { // unpacked rootfs takes seconds and deletes in readdir order, so an interrupt // part-way through would strip the rootfs while leaving image.json -- the // half-state that makes an image look cached and every later run fail. The -// rename is atomic, and a leftover `.old-` never satisfies a lookup, -// never appears in a listing, and is swept by the next pull or prune. +// rename is atomic, and a leftover `.old--` never satisfies a +// lookup and is swept by the next pull or prune. The directory's own lock +// keeps sweepers off it while the slow delete runs outside the store lock. func (s *Store) DeleteImage(digest string) error { if err := ValidateImageDigest(digest); err != nil { return err } + aside := s.displacedImageDir(digest) + held, err := s.displaceImage(digest, aside) + if err != nil { + return err + } + defer func() { _ = held.Close() }() + if err := os.RemoveAll(aside); err != nil { + return fmt.Errorf("failed to remove image %s: %w", digest, err) + } + return nil +} + +// displacedImageDir gives each commit or removal its own name, including +// concurrent operations in one process or in separate PID namespaces. +func (s *Store) displacedImageDir(digest string) string { + return filepath.Join(s.rootDir, "images", fmt.Sprintf("%s.old-%d-%s", digest, os.Getpid(), rand.Text())) +} + +// displaceImage is the locked part of DeleteImage. Its returned directory +// lock follows the image through the rename and protects the caller's delete. +func (s *Store) displaceImage(digest, aside string) (*StagingLock, error) { s.mu.Lock() defer s.mu.Unlock() - unlock, err := s.lockFile() if err != nil { - return err + return nil, err } defer unlock() - imageDir := s.ImageDir(digest) - if _, err := os.Stat(imageDir); err != nil { + held, err := lockDir(s.ImageDir(digest), false) + if err != nil { if os.IsNotExist(err) { - return fmt.Errorf("%w: %s", ErrImageNotFound, digest) + return nil, fmt.Errorf("%w: %s", ErrImageNotFound, digest) } - return fmt.Errorf("failed to read image directory: %w", err) + return nil, fmt.Errorf("failed to lock image directory: %w", err) } + if err := os.Rename(s.ImageDir(digest), aside); err != nil { + _ = held.Close() + return nil, fmt.Errorf("failed to displace image %s: %w", digest, err) + } + return held, nil +} - aside := filepath.Join(s.rootDir, "images", fmt.Sprintf("%s.old-%d", digest, os.Getpid())) - if err := os.Rename(imageDir, aside); err != nil { - return fmt.Errorf("failed to displace image %s: %w", digest, err) +// CommitImage publishes the image unpacked into stagingDir as digest, by +// swapping directories, never by deleting in place. +// +// RemoveAll on a populated rootfs takes seconds and deletes in readdir order, +// so an interrupt during it could strip the rootfs while leaving image.json -- +// exactly the half-state that made every later run fail. Renaming the +// previous image aside is atomic; the slow delete then happens once the new +// image is already published, where an interrupt is harmless: the leftover is +// swept by the next pull or prune once its directory lock is released. If +// publishing fails, the previous image is renamed back. +// +// The previous image is locked before it is renamed aside, and stays locked +// until it is deleted or put back: the lock follows the inode, so a sweeper +// finds the `.old--` held through rollback or deletion. Each +// commit uses a unique name so another commit in the same process can +// publish while this one is still deleting the displaced image. +// +// The swap happens under the store lock, as DeleteImage's does: another +// process reading the image -- a pull deciding whether it is whole, a prune +// deciding whether it is used -- must see it either entirely before or +// entirely after. The displaced image is deleted outside the store lock, +// so the lock is held for two renames and not for seconds. +// +// staging is the pull's lock on stagingDir, and is released the moment the +// rename publishes the directory, still inside the locked section. The lock +// follows the inode, so left to the caller it would hold the published image +// for as long as the caller took to let go -- through the delete of the +// displaced copy, seconds on a real image -- and another pull committing the +// same digest in that window would find the image busy and fail, over a +// directory nothing sweeps. +func (s *Store) CommitImage(digest, stagingDir string, staging *StagingLock) error { + if err := ValidateImageDigest(digest); err != nil { + return err } - if err := os.RemoveAll(aside); err != nil { - return fmt.Errorf("failed to remove image %s: %w", digest, err) + finalDir := s.ImageDir(digest) + oldDir := s.displacedImageDir(digest) + previous, err := s.swapImage(stagingDir, finalDir, oldDir, staging) + if err != nil { + return err + } + if previous != nil { + _ = os.RemoveAll(oldDir) + _ = previous.Close() } return nil } +// swapImage is the locked part of CommitImage. It returns the lock on the +// previous image when one was displaced to oldDir, and nil when there was +// none. +func (s *Store) swapImage(stagingDir, finalDir, oldDir string, staging *StagingLock) (*StagingLock, error) { + s.mu.Lock() + defer s.mu.Unlock() + unlock, err := s.lockFile() + if err != nil { + return nil, err + } + defer unlock() + + var previous *StagingLock + if _, err := os.Stat(finalDir); err == nil { + previous, err = lockDir(finalDir, false) + if err != nil { + return nil, fmt.Errorf("failed to displace the previous image: %w", err) + } + if err := os.Rename(finalDir, oldDir); err != nil { + _ = previous.Close() + return nil, fmt.Errorf("failed to displace the previous image: %w", err) + } + } + if err := os.Rename(stagingDir, finalDir); err != nil { + if previous != nil { + _ = os.Rename(oldDir, finalDir) // put the usable image back + _ = previous.Close() + } + return nil, fmt.Errorf("failed to commit image: %w", err) + } + _ = staging.Close() + return previous, nil +} + // StagingDirs lists the pull staging directories in images/. // // These are the `.tmp-` a pull unpacks into and the -// `.old-` it displaces a previous image to. Both are swept at the -// start of the next pull, which may never come; prune is the other sweeper. +// `.old--` it displaces a previous image to. Both are +// swept at the start of the next pull, which may never come; prune is the +// other sweeper. +// Either may be in use: see StagingInFlight. func (s *Store) StagingDirs() ([]string, error) { s.mu.RLock() defer s.mu.RUnlock() @@ -114,31 +216,57 @@ func (s *Store) StagingDirs() ([]string, error) { return names, nil } -// RemoveStagingDir removes one pull staging directory by name. +// RemoveStagingDir removes one pull staging directory by name, unless a pull +// is using it, which is ErrStagingInUse. It reports whether this call removed +// it: false with no error means it was already gone. // // Separate from DeleteImage because a staging name is not a valid digest, and // because the two mean different things to whoever is watching: one is an image // somebody pulled, the other is scaffolding from a pull that did not finish. -func (s *Store) RemoveStagingDir(name string) error { +// +// The directory's own lock is taken without waiting and held through the +// delete: a pull holds it for as long as it uses the directory, and the +// delete happening under the lock is what keeps a pull from adopting the +// name half-way through, and the other sweeper from deleting beside this +// one. The store lock is taken for the acquisition only, so a commit or an +// rmi mid-rename is not seen half-way, and released before the delete: a +// RemoveAll of a whole rootfs takes seconds, every other process's instance +// records go through that lock, and the directory lock is what excludes the +// others from this tree. +// +// A directory that is already gone is not an error. Prune lists, then +// removes, and a pull's sweep can take the leftover in between; the outcome +// prune wanted is there either way, and the report says which call got it. +func (s *Store) RemoveStagingDir(name string) (bool, error) { if name != filepath.Base(name) || !isStagingDir(name) { - return fmt.Errorf("%q is not a pull staging directory", name) + return false, fmt.Errorf("%q is not a pull staging directory", name) } + dir := filepath.Join(s.rootDir, "images", name) + l, err := s.lockStagingForRemoval(dir) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + return false, err + } + defer func() { _ = l.Close() }() + if err := os.RemoveAll(dir); err != nil { + return false, fmt.Errorf("failed to remove staging directory %s: %w", name, err) + } + return true, nil +} + +// lockStagingForRemoval takes a staging directory's lock without waiting, +// under the store lock, and hands it back with the store lock released. +func (s *Store) lockStagingForRemoval(dir string) (*StagingLock, error) { s.mu.Lock() defer s.mu.Unlock() - - // The store lock, as DeleteImage takes it: the in-process mutex covers one - // process, and this removes a directory another process may be committing - // a pull through. unlock, err := s.lockFile() if err != nil { - return err + return nil, err } defer unlock() - - if err := os.RemoveAll(filepath.Join(s.rootDir, "images", name)); err != nil { - return fmt.Errorf("failed to remove staging directory %s: %w", name, err) - } - return nil + return lockDir(dir, false) } // ImageDirNames lists every entry in images/ that is not pull scaffolding, diff --git a/pkg/store/image_test.go b/pkg/store/image_test.go index 5ab24fd..a5556f8 100644 --- a/pkg/store/image_test.go +++ b/pkg/store/image_test.go @@ -16,9 +16,13 @@ package store import ( "errors" + "fmt" "os" "path/filepath" + "strconv" "strings" + "sync" + "syscall" "testing" "time" ) @@ -114,7 +118,7 @@ func TestRemoveStagingDirRejectsAnythingElse(t *testing.T) { seedImage(t, s, testDigest, &ImageMetadata{Ref: "ghcr.io/x/y:v1"}) for _, bad := range []string{testDigest, "../instances", "images"} { - if err := s.RemoveStagingDir(bad); err == nil { + if _, err := s.RemoveStagingDir(bad); err == nil { t.Errorf("RemoveStagingDir(%q) was accepted", bad) } } @@ -140,7 +144,7 @@ func TestStagingDirsAndRemoval(t *testing.T) { t.Fatalf("StagingDirs = %v, want the two staging entries", staging) } for _, name := range staging { - if err := s.RemoveStagingDir(name); err != nil { + if _, err := s.RemoveStagingDir(name); err != nil { t.Fatalf("RemoveStagingDir(%q): %v", name, err) } } @@ -333,3 +337,169 @@ func TestFindImagesStillTreatsAShortIndexDigestAsAPrefix(t *testing.T) { t.Fatalf("short index digest: %d match(es), byPrefix %v", len(matches), byPrefix) } } + +// A commit publishes by rename: the previous image is displaced, the staging +// directory takes its place, and the displaced copy is gone afterwards. When +// the staging directory cannot be renamed in, the previous image is put back +// and nothing is lost. +func TestCommitImageSwapsAndRollsBack(t *testing.T) { + s := newTestStore(t) + imageDir := saveTestImage(t, s, "example.com/img:tag") + if err := os.WriteFile(filepath.Join(imageDir, "previous"), []byte("x"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + staging := filepath.Join(s.RootDir(), "images", testDigest+".tmp-"+strconv.Itoa(os.Getpid())) + held, err := CreateStaging(staging) + if err != nil { + t.Fatalf("CreateStaging: %v", err) + } + if err := os.MkdirAll(filepath.Join(staging, "rootfs"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + // A legacy leftover from this pid must not collide with the new + // displacement name. The ordinary staging sweep will collect it. + leftover := filepath.Join(s.RootDir(), "images", testDigest+".old-"+strconv.Itoa(os.Getpid())) + if err := os.MkdirAll(filepath.Join(leftover, "rootfs"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + if err := s.CommitImage(testDigest, staging, held); err != nil { + t.Fatalf("CommitImage: %v", err) + } + if _, err := os.Stat(filepath.Join(imageDir, "rootfs")); err != nil { + t.Errorf("the staging directory was not published: %v", err) + } + // The pull's lock followed the inode into the published image, and the + // commit let go of it there: another commit of the same digest must find + // the image free to displace, not busy. + if l, err := lockDir(imageDir, false); err != nil { + t.Errorf("the published image is still locked after the commit: %v", err) + } else { + _ = l.Close() + } + if _, err := os.Stat(filepath.Join(imageDir, "previous")); !os.IsNotExist(err) { + t.Errorf("the previous image is still in place: %v", err) + } + if _, err := os.Stat(leftover); err != nil { + t.Errorf("the commit touched an unrelated leftover: %v", err) + } + if _, err := s.SweepStaging(); err != nil { + t.Fatalf("SweepStaging: %v", err) + } + + // Rollback: nothing to rename in, so the image that was there comes back. + if err := s.CommitImage(testDigest, staging, nil); err == nil { + t.Fatal("CommitImage with no staging directory succeeded") + } + if _, err := os.Stat(filepath.Join(imageDir, "rootfs")); err != nil { + t.Errorf("the previous image was not put back after a failed commit: %v", err) + } + if _, err := os.Stat(leftover); !os.IsNotExist(err) { + t.Errorf("a failed commit left the previous image displaced: %v", err) + } + if err := s.CommitImage("not a digest", staging, nil); err == nil { + t.Error("CommitImage accepted a name that is not a digest") + } +} + +// A live operation with the same pid must not prevent another publication +// or removal. Legacy names can still exist during an upgrade. +func TestImageOperationsLeaveLockedLegacyLeftoverAlone(t *testing.T) { + for _, operation := range []string{"commit", "delete"} { + t.Run(operation, func(t *testing.T) { + s := newTestStore(t) + saveTestImage(t, s, "example.com/img:tag") + name := testDigest + ".old-" + strconv.Itoa(os.Getpid()) + leftover := plantStaging(t, s, name) + holdStaging(t, leftover) + var err error + if operation == "commit" { + staging := plantStaging(t, s, testDigest+".tmp-1") + err = s.CommitImage(testDigest, staging, holdStaging(t, staging)) + } else { + err = s.DeleteImage(testDigest) + } + if err != nil { + t.Fatalf("%s: %v", operation, err) + } + if _, err := os.Stat(filepath.Join(leftover, "rootfs", "layer")); err != nil { + t.Fatalf("%s touched the live leftover: %v", operation, err) + } + }) + } +} + +// Pause after the rmi rename, before its recursive delete. Prune must see +// the directory as busy, while other store users and commits can proceed. +func TestDisplacedImageRemovalHoldsOnlyDirectoryLock(t *testing.T) { + s := newTestStore(t) + saveTestImage(t, s, "example.com/img:tag") + aside := s.displacedImageDir(testDigest) + held, err := s.displaceImage(testDigest, aside) + if err != nil { + t.Fatalf("displaceImage: %v", err) + } + defer func() { _ = held.Close() }() + name := filepath.Base(aside) + if !s.StagingInFlight(name) { + t.Fatal("an active image removal is listed as abandoned") + } + if !s.mu.TryLock() { + t.Fatal("image removal still holds the store mutex") + } + s.mu.Unlock() + f, err := os.OpenFile(filepath.Join(s.RootDir(), ".lock"), os.O_RDWR, 0600) + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + t.Fatalf("image removal still holds the store flock: %v", err) + } + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + if removed, err := s.RemoveStagingDir(name); removed || !errors.Is(err, ErrStagingInUse) { + t.Fatalf("RemoveStagingDir = %v, %v; want false, ErrStagingInUse", removed, err) + } + staging := plantStaging(t, s, testDigest+".tmp-1") + if err := s.CommitImage(testDigest, staging, holdStaging(t, staging)); err != nil { + t.Fatalf("commit during image removal: %v", err) + } + if err := s.DeleteImage(testDigest); err != nil { + t.Fatalf("second removal: %v", err) + } + if _, err := os.Stat(aside); err != nil { + t.Fatalf("another operation touched the first removal: %v", err) + } + _ = held.Close() + if removed, err := s.RemoveStagingDir(name); err != nil || !removed { + t.Fatalf("cleanup after release = %v, %v", removed, err) + } +} + +func TestConcurrentCommitsInOneProcess(t *testing.T) { + s := newTestStore(t) + saveTestImage(t, s, "example.com/img:tag") + var wg sync.WaitGroup + start := make(chan struct{}) + for worker := range 2 { + wg.Go(func() { + <-start + for iteration := range 25 { + name := fmt.Sprintf("%s.tmp-%d-%d", testDigest, worker, iteration) + staging := plantStaging(t, s, name) + if err := s.CommitImage(testDigest, staging, holdStaging(t, staging)); err != nil { + t.Errorf("CommitImage: %v", err) + return + } + } + }) + } + close(start) + wg.Wait() + if _, err := os.Stat(filepath.Join(s.ImageDir(testDigest), "rootfs", "layer")); err != nil { + t.Fatalf("published image missing: %v", err) + } + if names, err := s.StagingDirs(); err != nil || len(names) != 0 { + t.Fatalf("commits left staging directories: %v, %v", names, err) + } +} diff --git a/pkg/store/integrity.go b/pkg/store/integrity.go new file mode 100644 index 0000000..9dd9059 --- /dev/null +++ b/pkg/store/integrity.go @@ -0,0 +1,212 @@ +// Copyright (c) 2026, NOFire AI +// +// 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 store + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "sort" + "strconv" + "strings" +) + +// ErrImageDamaged says a cached image is not what its pull published. +var ErrImageDamaged = errors.New("image is damaged") + +// rootfsEntriesFile holds the number of entries under rootfs/ at the moment +// the image was published. It is what lets a later pull tell an image that +// has lost part of its tree since from one that is still what was published. +const rootfsEntriesFile = "rootfs-entries" + +// CountRootfsEntries counts every entry under rootfs/ in imageDir, the +// directory itself excluded. +// +// Nothing writes into a published rootfs: `hull run` shares it read-only or +// clones it into the instance, so the count a pull records holds for as long +// as the image does. +// +// A directory that cannot be listed counts as one entry and its contents as +// none. Unpack puts the image's own mode on every directory it ships, and an +// image is free to ship one its owner cannot read; the walk sees the same +// mode when the count is recorded and when it is checked, so the two agree, +// and a pull is not failed over a count. Only a rootfs that is not there at +// all is an error. +func CountRootfsEntries(imageDir string) (int, error) { + root := filepath.Join(imageDir, "rootfs") + n := -1 // the root is visited first and does not count + err := filepath.WalkDir(root, func(path string, _ fs.DirEntry, err error) error { + if err != nil { + if path == root { + return err + } + // WalkDir has already called back for this directory once, + // without an error, and it was counted then. + return fs.SkipDir + } + n++ + return nil + }) + if err != nil { + return 0, err + } + return n, nil +} + +// WriteRootfsEntries records the rootfs entry count in imageDir. Call it on +// the staging directory, after the last layer is unpacked and before the +// rename that publishes it, so a published image always carries the count it +// was published with. +func WriteRootfsEntries(imageDir string) error { + n, err := CountRootfsEntries(imageDir) + if err != nil { + return fmt.Errorf("failed to count the rootfs entries: %w", err) + } + p := filepath.Join(imageDir, rootfsEntriesFile) + if err := os.WriteFile(p, []byte(strconv.Itoa(n)+"\n"), 0600); err != nil { + return fmt.Errorf("failed to record the rootfs entry count: %w", err) + } + return nil +} + +// readRootfsEntries returns the recorded count, and false when the image +// carries none, which is what an image published before the count existed +// looks like. +func readRootfsEntries(imageDir string) (int, bool, error) { + data, err := os.ReadFile(filepath.Join(imageDir, rootfsEntriesFile)) + if err != nil { + if os.IsNotExist(err) { + return 0, false, nil + } + return 0, false, err + } + n, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil || n < 0 { + return 0, false, fmt.Errorf("the recorded rootfs entry count %q is not a count", strings.TrimSpace(string(data))) + } + return n, true, nil +} + +// VerifyImage checks a cached image against what its pull recorded, and +// returns an error wrapping ErrImageDamaged when the rootfs has lost entries +// since. +// +// ImageComplete says the pieces are there; this says the tree is still the +// size it was published at. An image can lose entries after publication and +// look no different from here: a sweep's RemoveAll that was still running +// through the staging directory when the rename published it, or anything +// else that deletes under images/. Every later lookup found such an image +// complete and reused it, and `hull rmi` was the only way out. The pull path +// checks this before deciding a digest it already holds needs no unpack, so +// one `hull pull` repairs it instead. +// +// What the count cannot tell is a tree that was already short when it was +// counted: it is taken after the last layer is unpacked, so an unpack that +// lost entries along the way records the tree it ended up with. That is the +// sweep race, and the sweep is what was fixed; this is the check for what +// happens to an image afterwards. +// +// Only a lower count is damage. Losing part of the tree is the one failure +// this exists for, and it can only take entries away; an entry added beside +// the image's own -- a .DS_Store from a Finder window, say -- is not that, +// and re-unpacking a whole image over it would be the wrong answer. +// +// An image that recorded no count cannot be checked and passes: it was +// published by a hull from before the count existed, and refusing it would +// re-unpack every image in every existing store. +// +// The walk runs without the store lock, since it is on the `--pull=always` +// path and every other process's instance records go through that lock; a +// walk of twenty thousand entries with it held would stall a `hull ps` next +// door. What the lock is for is a commit of the same digest in another +// process: its swap happens under the lock, and a walk that saw the swap +// half-way would read a whole image as damaged, or not find the rootfs at +// all. So a shortfall, and a walk that failed, are confirmed under the lock, +// by reading the count and walking again; only what holds with nothing +// swapping is the answer. +func (s *Store) VerifyImage(digest string) error { + if err := ValidateImageDigest(digest); err != nil { + return err + } + imageDir := s.ImageDir(digest) + short, err := s.rootfsShort(imageDir) + if err == nil && short == nil { + return nil + } + + s.mu.RLock() + defer s.mu.RUnlock() + unlock, err := s.lockFile() + if err != nil { + return err + } + defer unlock() + short, err = s.rootfsShort(imageDir) + if err != nil { + return err + } + return short +} + +// rootfsShort compares the rootfs against its recorded count, and returns +// the ErrImageDamaged error for a shortfall, nil for a tree that is whole or +// unrecorded, and any other error for a record or a tree that could not be +// read. +func (s *Store) rootfsShort(imageDir string) (short error, err error) { + recorded, ok, err := readRootfsEntries(imageDir) + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrImageDamaged, err) + } + if !ok { + return nil, nil + } + found, err := CountRootfsEntries(imageDir) + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrImageDamaged, err) + } + if found < recorded { + return fmt.Errorf("%w: rootfs holds %d entries, %d were recorded when it was pulled", + ErrImageDamaged, found, recorded), nil + } + return nil, nil +} + +// ImageHolders returns the records of the instances that name the digest, +// running or stopped, sorted by id. +// +// This is what `hull rmi` and `hull prune` refuse to remove an image over, +// and what a pull that would displace an image has to check first: a live +// instance shares the image's rootfs with its guest, and a stopped one +// expects it back on the next start. Replacing the directory under either +// is not a repair. Which records are live is the caller's to decide: a +// record's status alone is not it, since a death after the pid write +// leaves one at "running" for good, and the check that tells a VMM from a +// recycled pid lives with the commands. +func (s *Store) ImageHolders(digest string) ([]*InstanceState, error) { + instances, err := s.ListInstances() + if err != nil { + return nil, err + } + var holders []*InstanceState + for _, inst := range instances { + if inst.ImageDigest == digest { + holders = append(holders, inst) + } + } + sort.Slice(holders, func(i, j int) bool { return holders[i].ID < holders[j].ID }) + return holders, nil +} diff --git a/pkg/store/integrity_test.go b/pkg/store/integrity_test.go new file mode 100644 index 0000000..39c91bc --- /dev/null +++ b/pkg/store/integrity_test.go @@ -0,0 +1,189 @@ +// Copyright (c) 2026, NOFire AI +// +// 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 store + +import ( + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +// publishTestRootfs writes a small rootfs into the test image and records its +// entry count, the way a pull publishes one. +func publishTestRootfs(t *testing.T, s *Store) string { + t.Helper() + imageDir := saveTestImage(t, s, "example.com/img:tag") + for _, p := range []string{"usr/bin/bash", "usr/bin/sh", "etc/passwd"} { + full := filepath.Join(imageDir, "rootfs", p) + if err := os.MkdirAll(filepath.Dir(full), 0755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(full, []byte("x"), 0644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + } + if err := WriteUnpackSchema(imageDir); err != nil { + t.Fatalf("WriteUnpackSchema: %v", err) + } + if err := WriteRootfsEntries(imageDir); err != nil { + t.Fatalf("WriteRootfsEntries: %v", err) + } + return imageDir +} + +// The second half of #114: a pull whose staging directory was deleted under +// it published an image with part of its tree missing, and every later lookup +// found it complete. The recorded count is what tells the two apart. +func TestVerifyImageDetectsAMissingEntry(t *testing.T) { + s := newTestStore(t) + imageDir := publishTestRootfs(t, s) + + if n, err := CountRootfsEntries(imageDir); err != nil || n != 6 { + t.Fatalf("CountRootfsEntries = %d, %v; want 6 (usr, usr/bin, two binaries, etc, passwd)", n, err) + } + if err := s.VerifyImage(testDigest); err != nil { + t.Fatalf("a whole image failed verification: %v", err) + } + + if err := os.Remove(filepath.Join(imageDir, "rootfs", "usr", "bin", "bash")); err != nil { + t.Fatalf("Remove: %v", err) + } + if !s.ImageComplete(testDigest) { + t.Fatal("precondition: the truncated image still looks complete, which is the bug") + } + err := s.VerifyImage(testDigest) + if !errors.Is(err, ErrImageDamaged) { + t.Fatalf("VerifyImage on a truncated rootfs = %v, want ErrImageDamaged", err) + } +} + +// An image from before the count existed carries none, and must keep passing: +// failing it would re-unpack every image in every existing store on upgrade. +func TestVerifyImageTrustsAnUnrecordedImage(t *testing.T) { + s := newTestStore(t) + imageDir := publishTestRootfs(t, s) + if err := os.Remove(filepath.Join(imageDir, rootfsEntriesFile)); err != nil { + t.Fatalf("Remove: %v", err) + } + if err := os.Remove(filepath.Join(imageDir, "rootfs", "etc", "passwd")); err != nil { + t.Fatalf("Remove: %v", err) + } + if err := s.VerifyImage(testDigest); err != nil { + t.Errorf("an image with no recorded count was refused: %v", err) + } +} + +// A count that cannot be read is not a count of zero, and not a pass either. +func TestVerifyImageRefusesADamagedCount(t *testing.T) { + s := newTestStore(t) + imageDir := publishTestRootfs(t, s) + for _, stamp := range []string{"", "garbage", "-1", "3 4"} { + if err := os.WriteFile(filepath.Join(imageDir, rootfsEntriesFile), []byte(stamp), 0600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := s.VerifyImage(testDigest); !errors.Is(err, ErrImageDamaged) { + t.Errorf("stamp %q: VerifyImage = %v, want ErrImageDamaged", stamp, err) + } + } +} + +// An image is free to ship a directory its owner cannot list (unpack applies +// the image's own modes), and the count must neither fail the pull over it +// nor disagree with itself later. +func TestCountRootfsEntriesSkipsAnUnlistableDirectory(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root lists every directory, so there is nothing to skip") + } + s := newTestStore(t) + imageDir := publishTestRootfs(t, s) + locked := filepath.Join(imageDir, "rootfs", "etc", "private") + if err := os.MkdirAll(locked, 0755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Join(locked, "key"), []byte("x"), 0600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.Chmod(locked, 0); err != nil { + t.Fatalf("Chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(locked, 0755) }) + + // The directory counts; what it hides does not. The other six are the + // tree publishTestRootfs wrote. + n, err := CountRootfsEntries(imageDir) + if err != nil { + t.Fatalf("CountRootfsEntries: %v", err) + } + if n != 7 { + t.Errorf("CountRootfsEntries = %d, want 7", n) + } + if err := WriteRootfsEntries(imageDir); err != nil { + t.Fatalf("WriteRootfsEntries: %v", err) + } + if err := s.VerifyImage(testDigest); err != nil { + t.Errorf("an image with an unlistable directory failed verification: %v", err) + } +} + +// Only a rootfs that is not there is an error. +func TestCountRootfsEntriesFailsWithoutARootfs(t *testing.T) { + s := newTestStore(t) + imageDir := saveTestImage(t, s, "example.com/img:tag") + if _, err := CountRootfsEntries(imageDir); err == nil { + t.Error("CountRootfsEntries on a missing rootfs returned no error") + } +} + +// An entry added beside the image's own is not a truncation. A Finder window +// leaves a .DS_Store in any directory it shows, and that must not cost a +// re-unpack of the whole image. +func TestVerifyImageToleratesAnExtraEntry(t *testing.T) { + s := newTestStore(t) + imageDir := publishTestRootfs(t, s) + if err := os.WriteFile(filepath.Join(imageDir, "rootfs", ".DS_Store"), []byte("x"), 0644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := s.VerifyImage(testDigest); err != nil { + t.Errorf("an extra entry was read as damage: %v", err) + } +} + +func TestImageHolders(t *testing.T) { + s := newTestStore(t) + for _, inst := range []struct{ id, digest, status string }{ + {"b", testDigest, "running"}, {"a", testDigest, "stopped"}, {"other", "sha256:" + strings.Repeat("0", 64), "running"}, + } { + if _, err := s.CreateInstance(inst.id); err != nil { + t.Fatalf("CreateInstance: %v", err) + } + if err := s.SaveInstance(&InstanceState{ID: inst.id, ImageDigest: inst.digest, Status: inst.status}); err != nil { + t.Fatalf("SaveInstance: %v", err) + } + } + holders, err := s.ImageHolders(testDigest) + if err != nil { + t.Fatalf("ImageHolders: %v", err) + } + var got []string + for _, h := range holders { + got = append(got, h.ID+"="+h.Status) + } + if want := []string{"a=stopped", "b=running"}; !reflect.DeepEqual(got, want) { + t.Errorf("ImageHolders = %v, want %v", got, want) + } +} diff --git a/pkg/store/staging.go b/pkg/store/staging.go new file mode 100644 index 0000000..fa416a3 --- /dev/null +++ b/pkg/store/staging.go @@ -0,0 +1,243 @@ +// Copyright (c) 2026, NOFire AI +// +// 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 store + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + "syscall" +) + +// Pull staging directories, and the locks that say whose they are. +// +// A pull unpacks into `.tmp-` and displaces the previous image +// to `.old--` while it commits. Both are swept by the +// next pull into the store and by `hull prune`, and neither may remove one a +// pull is still using: that fails the pull, or publishes an image missing the +// layers that were deleted under it (#114). +// +// So the pull holds a lock on each directory it is using, a flock on the +// directory's own descriptor, for as long as it is using it. The kernel +// releases the lock when the process exits, however it exits, and the lock +// follows the inode through a rename, so the displaced image stays covered +// from the rename aside to the delete. A sweeper tries the lock without +// waiting: busy means a pull is using the directory, acquired means nothing +// is, and the sweeper deletes while holding it, so two sweepers cannot race +// each other and a pull cannot adopt the directory half-way through. +// +// The pid in the name only keeps two pulls' directories apart. Prune used to +// read liveness from it, together with how recently the tree was written, and +// both were guesses: pids are recycled, and a layer's tar restores the mtimes +// of what it unpacks, so a slow pull looked abandoned an hour in. + +// ErrStagingInUse says a pull holds the staging directory. +var ErrStagingInUse = errors.New("a pull is using the staging directory") + +// StagingLock is an exclusive lock on a directory, held through its open +// descriptor. Close releases it; so does the death of the process. +type StagingLock struct { + f *os.File +} + +// lockDir takes the lock on dir, waiting for it or not. Busy without waiting +// is ErrStagingInUse. +func lockDir(dir string, wait bool) (*StagingLock, error) { + f, err := os.Open(dir) + if err != nil { + return nil, err + } + how := syscall.LOCK_EX + if !wait { + how |= syscall.LOCK_NB + } + if err := syscall.Flock(int(f.Fd()), how); err != nil { + _ = f.Close() + if errors.Is(err, syscall.EWOULDBLOCK) { + return nil, fmt.Errorf("%w: %s", ErrStagingInUse, filepath.Base(dir)) + } + return nil, fmt.Errorf("failed to lock %s: %w", dir, err) + } + return &StagingLock{f: f}, nil +} + +// Close releases the lock. Closing twice is harmless. +func (l *StagingLock) Close() error { + if l == nil || l.f == nil { + return nil + } + f := l.f + l.f = nil + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + return f.Close() +} + +// CreateStaging creates a staging directory and returns it locked. +// +// The lock is on an inode and the name is what the pull goes on to use, so +// the two have to be checked against each other: a sweeper can take and +// delete the directory between the mkdir and the lock, and a pull that kept +// writing into a path the sweeper had just removed would be the bug again. +// The lock waits for a sweeper or a same-name pull (in this process or +// another PID namespace), which can take as long as that pull runs. Then +// the path is checked to still name the inode locked, and the directory +// is made again if not. +func CreateStaging(dir string) (*StagingLock, error) { + return createStaging(dir, lockDir) +} + +// The lock function is passed in so tests can reproduce a sweep between +// creation and lock acquisition without timing-dependent goroutines. +func createStaging(dir string, lock func(string, bool) (*StagingLock, error)) (*StagingLock, error) { + for range 3 { + if err := os.MkdirAll(dir, 0700); err != nil { + return nil, fmt.Errorf("failed to create the staging directory: %w", err) + } + l, err := lock(dir, true) + if err != nil { + if os.IsNotExist(err) { + continue + } + return nil, err + } + if sameInode(l.f, dir) { + return l, nil + } + _ = l.Close() + } + return nil, fmt.Errorf("failed to keep the staging directory %s: it was swept each time it was made", dir) +} + +// sameInode reports whether path still names the directory f holds open. +func sameInode(f *os.File, path string) bool { + held, err := f.Stat() + if err != nil { + return false + } + named, err := os.Stat(path) + if err != nil { + return false + } + return os.SameFile(held, named) +} + +// StagingInFlight reports whether a pull is using a staging directory in +// images/: whether its lock is held. A name nothing is listed under is not +// in flight. +// +// This answers for a listing. A removal does not trust it, because the pull +// could start between the question and the delete; RemoveStagingDir takes +// the lock itself and holds it while it deletes. +func (s *Store) StagingInFlight(name string) bool { + l, err := lockDir(filepath.Join(s.rootDir, "images", name), false) + if err != nil { + return errors.Is(err, ErrStagingInUse) + } + _ = l.Close() + return false +} + +// Displaced reports whether a staging name is the `.old-*` kind. +func Displaced(name string) bool { + return strings.Contains(filepath.Base(name), ".old-") +} + +// SweepStaging removes the staging directories in images/ no pull is using, +// and reports which it removed. +// +// The pull path calls this before unpacking, so a store does not accumulate +// the leftovers of interrupted pulls until somebody runs `hull prune`. It +// used to delete every staging directory it found, which took the +// `.tmp-` a concurrent pull was still unpacking into: that pull +// then either failed on the missing tree or, when the deletion landed between +// two layers, finished unpacking the remaining ones into a fresh directory and +// published an image with the earlier layers' entries missing. Every later +// pull and run found that image complete and reused it. +// +// The `.old-*` kind has the same shape. A commit renames the previous +// image aside, and renames it back if publishing the new one fails; a sweep +// in between left that rollback nothing to restore. +// +// Nothing in a sweep is unrecoverable: a staging directory nothing holds +// belongs to a pull that will not finish, and the image it was for can be +// pulled again. +func (s *Store) SweepStaging() ([]string, error) { + names, err := s.StagingDirs() + if err != nil { + return nil, err + } + var removed []string + var errs []error + for _, name := range names { + got, err := s.RemoveStagingDir(name) + switch { + case err == nil && got: + removed = append(removed, name) + case err == nil: + // Another sweeper got there first. + case errors.Is(err, ErrStagingInUse): + // A pull is using it. + default: + errs = append(errs, err) + } + } + return removed, errors.Join(errs...) +} + +// DirDiskUsage sums what a directory tree actually occupies, so the figure +// printed is the space the host gets back rather than the size of the layers +// the image was pulled from. +// +// Blocks, not sizes: an unpacked rootfs is mostly small files, each rounded up +// to a block, and a sparse file occupies less than it claims. Hard links are +// counted once -- unpack creates them for repeated layer entries, and counting +// each name would overstate the total. +// +// APFS clones are counted in full, since each clone reports the blocks it +// shares. Nothing hull writes under images/ is a clone of another entry there, +// so the figure is exact for a store hull built and an upper bound for one +// somebody has copied inside with `cp -c`. +// +// Returns what it managed to measure. A walk that fails part-way gives a low +// figure in a report; refusing to delete over it would be worse. +func DirDiskUsage(dir string) int64 { + var total int64 + seen := make(map[uint64]bool) + _ = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return nil + } + info, err := d.Info() + if err != nil { + return nil + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return nil + } + if st.Nlink > 1 { + if seen[st.Ino] { + return nil + } + seen[st.Ino] = true + } + total += st.Blocks * 512 + return nil + }) + return total +} diff --git a/pkg/store/staging_test.go b/pkg/store/staging_test.go new file mode 100644 index 0000000..de5064b --- /dev/null +++ b/pkg/store/staging_test.go @@ -0,0 +1,282 @@ +// Copyright (c) 2026, NOFire AI +// +// 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 store + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "reflect" + "sort" + "strings" + "testing" +) + +const stagingTestDigest = "sha256:c408baae42f5c74c0661fbc20a289fd23d4322988e52c88cd54108e5c4c74893" + +// plantStaging writes a staging directory with one file under rootfs/, the +// way a pull in flight has one, and returns its path. +func plantStaging(t *testing.T, s *Store, name string) string { + t.Helper() + dir := filepath.Join(s.RootDir(), "images", name) + if err := os.MkdirAll(filepath.Join(dir, "rootfs"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "rootfs", "layer"), []byte("x"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + return dir +} + +// holdStaging takes the lock a pull holds on its staging directory, and +// releases it when the test ends. +func holdStaging(t *testing.T, dir string) *StagingLock { + t.Helper() + l, err := lockDir(dir, false) + if err != nil { + t.Fatalf("lockDir: %v", err) + } + t.Cleanup(func() { _ = l.Close() }) + return l +} + +// The figure printed is the space the host gets back, so it counts blocks and +// counts a hard-linked file once. Unpack creates those for repeated entries. +func TestDirDiskUsageCountsAHardLinkOnce(t *testing.T) { + dir := t.TempDir() + body := bytes.Repeat([]byte("x"), 64*1024) + first := filepath.Join(dir, "a") + if err := os.WriteFile(first, body, 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + single := DirDiskUsage(dir) + if single < int64(len(body)) { + t.Fatalf("DirDiskUsage = %d for a %d-byte file", single, len(body)) + } + if err := os.Link(first, filepath.Join(dir, "b")); err != nil { + t.Fatalf("Link: %v", err) + } + if got := DirDiskUsage(dir); got != single { + t.Errorf("a hard link changed the total from %d to %d", single, got) + } +} + +// The bug in #114: the sweep a pull runs before unpacking removed every +// staging directory it found, including the one a concurrent pull was still +// unpacking into and the previous image another was holding aside to roll +// back to. A pull holds a lock on each; both must survive, and the leftovers +// of a pull that is gone -- whose lock died with it -- must not. +func TestSweepStagingLeavesALockedDirectoryAlone(t *testing.T) { + s := newTestStore(t) + live := []string{stagingTestDigest + ".tmp-1", stagingTestDigest + ".old-1"} + gone := []string{stagingTestDigest + ".tmp-2", stagingTestDigest + ".old-2"} + for _, name := range live { + holdStaging(t, plantStaging(t, s, name)) + } + for _, name := range gone { + plantStaging(t, s, name) + } + for _, name := range live { + if !s.StagingInFlight(name) { + t.Errorf("%s is locked and not in flight", name) + } + } + for _, name := range gone { + if s.StagingInFlight(name) { + t.Errorf("%s is unlocked and in flight", name) + } + } + + removed, err := s.SweepStaging() + if err != nil { + t.Fatalf("SweepStaging: %v", err) + } + sort.Strings(removed) + sort.Strings(gone) + if !reflect.DeepEqual(removed, gone) { + t.Errorf("SweepStaging removed %v, want %v", removed, gone) + } + for _, name := range live { + if _, err := os.Stat(filepath.Join(s.RootDir(), "images", name, "rootfs", "layer")); err != nil { + t.Errorf("the sweep took a live pull's %s: %v", name, err) + } + } + for _, name := range gone { + if _, err := os.Stat(filepath.Join(s.RootDir(), "images", name)); !os.IsNotExist(err) { + t.Errorf("%s of a dead pull survived the sweep: %v", name, err) + } + } +} + +// A removal does not trust a listing: it takes the lock itself and refuses +// when a pull holds it, so a pull that started between the two is safe. +func TestRemoveStagingDirRefusesALockedDirectory(t *testing.T) { + s := newTestStore(t) + name := stagingTestDigest + ".tmp-1" + l := holdStaging(t, plantStaging(t, s, name)) + if _, err := s.RemoveStagingDir(name); !errors.Is(err, ErrStagingInUse) { + t.Fatalf("RemoveStagingDir on a locked directory = %v, want ErrStagingInUse", err) + } + _ = l.Close() + if got, err := s.RemoveStagingDir(name); err != nil || !got { + t.Fatalf("RemoveStagingDir once released = %v, %v; want true, nil", got, err) + } + if s.StagingInFlight(name) { + t.Error("a removed directory reads as in flight") + } +} + +// CreateStaging hands back the directory locked, and a sweep cannot take it +// from under the pull. Releasing the lock is what makes it sweepable. +func TestCreateStagingIsHeldUntilClosed(t *testing.T) { + s := newTestStore(t) + name := stagingTestDigest + ".tmp-1" + dir := filepath.Join(s.RootDir(), "images", name) + l, err := CreateStaging(dir) + if err != nil { + t.Fatalf("CreateStaging: %v", err) + } + if removed, err := s.SweepStaging(); err != nil || len(removed) != 0 { + t.Fatalf("SweepStaging took a directory a pull just created: %v, %v", removed, err) + } + if err := l.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if err := l.Close(); err != nil { + t.Fatalf("second Close: %v", err) + } + if removed, err := s.SweepStaging(); err != nil || len(removed) != 1 { + t.Fatalf("SweepStaging after release: %v, %v", removed, err) + } +} + +// The lock is on the inode and the name is what the pull uses, so they are +// checked against each other: a directory swept between the mkdir and the +// lock is made again, and the pull ends up holding the one it will write to. +func TestCreateStagingKeepsTheNameItLocked(t *testing.T) { + s := newTestStore(t) + name := stagingTestDigest + ".tmp-1" + dir := filepath.Join(s.RootDir(), "images", name) + l, err := CreateStaging(dir) + if err != nil { + t.Fatalf("CreateStaging: %v", err) + } + if !sameInode(l.f, dir) { + t.Error("the lock held is not on the directory named") + } + // What a sweep looks like from the pull's side: the path no longer + // names the inode held. + if err := os.Rename(dir, dir+".swept"); err != nil { + t.Fatalf("Rename: %v", err) + } + if sameInode(l.f, dir) { + t.Error("a missing path still matched the inode held") + } + _ = l.Close() +} + +// Prune lists, then removes, and a pull's sweep can take the leftover in +// between. The outcome prune wanted is there either way, so a directory that +// is already gone is not a failed removal. +func TestRemoveStagingDirAcceptsAGoneDirectory(t *testing.T) { + s := newTestStore(t) + got, err := s.RemoveStagingDir(stagingTestDigest + ".tmp-1") + if err != nil { + t.Fatalf("RemoveStagingDir on a directory that is not there: %v", err) + } + if got { + t.Error("RemoveStagingDir claims to have removed a directory that was not there") + } +} + +// Exercise both sweep windows: before the directory is opened, and after +// its inode is opened but before the creator checks the name it will use. +func TestCreateStagingRetriesAfterSweep(t *testing.T) { + for _, when := range []string{"before open", "after open", "replaced inode"} { + t.Run(when, func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "staging") + attempts := 0 + var discarded *StagingLock + held, err := createStaging(dir, func(dir string, wait bool) (*StagingLock, error) { + attempts++ + if attempts != 1 { + return lockDir(dir, wait) + } + if when != "before open" { + var err error + discarded, err = lockDir(dir, wait) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = discarded.Close() }) + } + if err := os.RemoveAll(dir); err != nil { + t.Fatal(err) + } + if when == "replaced inode" { + if err := os.Mkdir(dir, 0700); err != nil { + t.Fatal(err) + } + } + if discarded != nil { + return discarded, nil + } + return lockDir(dir, wait) + }) + if err != nil { + t.Fatal(err) + } + defer func() { _ = held.Close() }() + if attempts != 2 || !sameInode(held.f, dir) { + t.Fatalf("after %d attempts the lock must name the surviving directory", attempts) + } + if discarded != nil && discarded.f != nil { + t.Error("retry leaked the discarded inode's lock") + } + if other, err := lockDir(dir, false); !errors.Is(err, ErrStagingInUse) { + _ = other.Close() + t.Fatalf("returned directory is not locked: %v", err) + } + }) + } +} + +func TestCreateStagingStopsAfterRepeatedSweeps(t *testing.T) { + dir := filepath.Join(t.TempDir(), "staging") + attempts := 0 + held, err := createStaging(dir, func(dir string, wait bool) (*StagingLock, error) { + attempts++ + if err := os.RemoveAll(dir); err != nil { + t.Fatal(err) + } + return lockDir(dir, wait) + }) + if held != nil || err == nil || attempts != 3 || !strings.Contains(err.Error(), "swept each time") { + t.Fatalf("CreateStaging = %v, %v after %d attempts", held, err, attempts) + } +} + +func TestCreateStagingReturnsLockFailure(t *testing.T) { + want := errors.New("lock failed") + attempts := 0 + held, err := createStaging(filepath.Join(t.TempDir(), "staging"), func(string, bool) (*StagingLock, error) { + attempts++ + return nil, want + }) + if held != nil || !errors.Is(err, want) || attempts != 1 { + t.Fatalf("CreateStaging = %v, %v after %d attempts", held, err, attempts) + } +} diff --git a/pkg/store/store.go b/pkg/store/store.go index ae4966a..f307cdf 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -269,8 +269,8 @@ func WriteImageMetadata(dir string, metadata *ImageMetadata) error { // isStagingDir reports whether an images/ entry is pull scaffolding rather than // a published image. The pull path stages under .tmp- and displaces -// the previous image to .old-; both now carry an image.json, so -// name is the only thing that distinguishes them from the real entry. +// the previous image to .old--. Both carry image.json, +// so the name is what distinguishes them from a published image. func isStagingDir(name string) bool { return strings.Contains(name, ".tmp-") || strings.Contains(name, ".old-") }