carlos / platform Public

hostfiles: Restore removes a tombstone past the cap in one linear, cancellable walk (Astra review round 2, P2): skipping it left a deep file that made the next scan refuse the tree and stranded relocation's flush

WIP. Not pushed: the Plan 14 root-read census needs a ruling for the new
unit restore_unix.go:removePastCap (file:Open@94, root.Open(".") to get a
descriptor for the restore root). The census's envelope overlay is not
updated yet.

The walk:
- descendCtx: dir_unix.go's descend with a ctx check per component.
- removePastCap: opens each component relative to the last with
  O_NOFOLLOW|O_DIRECTORY, unlinkats the name, and fsyncs the parent
  through its held descriptor.
- Deep tombstones are never mark()ed. Under-cap tombstones are unchanged
  (removeInside).
- restore_other.go is the !unix stub, which errors.

Tests:
- A 2048-level removal takes 22-25ms; a cubic removeInside is still
  running at 10s.
- Cancellation stops mid-walk.
- A symlink is never followed, at depths 0, 1024 and 2047.
- Restore, then ReleaseFlushFiles.Flush, commits.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude agent, operated by paul@keymail.dev 3ab48ff700c3876ae9f33211427d855600387cf3 parent a7f173c
6 files changed, +408 −28
  • internal/host/relocationflush_deeptombstone_linux_test.go +111 −0
  • internal/hostfiles/dir_unix.go +14 −0
  • internal/hostfiles/fix5_linux_test.go +171 −17
  • internal/hostfiles/restore.go +15 −11
  • internal/hostfiles/restore_other.go +17 −0
  • internal/hostfiles/restore_unix.go +80 −0
diff --git a/internal/host/relocationflush_deeptombstone_linux_test.go b/internal/host/relocationflush_deeptombstone_linux_test.go
new file mode 100644
index 000000000..3ce322a32
--- /dev/null
+++ b/internal/host/relocationflush_deeptombstone_linux_test.go
@@ -0,0 +1,111 @@
+//go:build linux
+
+package host
+
+import (
+ "context"
+ "encoding/json"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/hostclaim"
+ "github.com/carlosframework/platform/internal/hostfiles"
+ "github.com/carlosframework/platform/internal/moves"
+ "github.com/carlosframework/platform/internal/registry"
+ "github.com/carlosframework/platform/internal/release"
+)
+
+// Astra's review of the hostfiles path cap, round 2: a manifest written
+// before the cap may tombstone a path past it whose file still stands at
+// the destination. Restore removes it (in one linear walk), so the release
+// flush's scan does not refuse the tree (ErrTooDeep) and the flush, which
+// EndProbe requires, commits.
+func TestReleaseFlushCommitsAfterARestoreOfATombstonePastTheCap(t *testing.T) {
+ ctx := context.Background()
+ const host = "shop.example"
+ holder := &hostclaim.Holder{Location: "instances/" + host + "/inc", Incarnation: "inc-1", Session: 6, Generation: "g6"}
+ prefix := moves.GenerationFilesPrefix(holder.Location, holder.Generation)
+ data := release.NewDirStore(t.TempDir())
+ reg, err := registry.Open(filepath.Join(t.TempDir(), "registry.db"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ t.Cleanup(func() { reg.Close() })
+ if err := reg.Upsert(registry.Route{Host: host, Kind: "service", Addr: "10.0.0.1:3000", Status: "ready", Hibernate: true}); err != nil {
+ t.Fatal(err)
+ }
+ dir, envDir := t.TempDir(), t.TempDir()
+ if err := os.WriteFile(filepath.Join(envDir, host+".env"), []byte("A=1\n"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+
+ // The destination: a file past the cap, its parent exactly at it.
+ r, err := os.OpenRoot(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var parts []string
+ for n := 0; n < hostfiles.MaxPathBytes/2+1; n++ {
+ if err := r.Mkdir("d", 0o755); err != nil {
+ t.Fatal(err)
+ }
+ next, err := r.OpenRoot("d")
+ r.Close()
+ if err != nil {
+ t.Fatal(err)
+ }
+ r, parts = next, append(parts, "d")
+ }
+ if err := r.WriteFile("f", []byte("deep"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ r.Close()
+ deep := strings.Join(append(parts, "f"), "/")
+
+ // The inherited manifest: one healthy file and the deep tombstone.
+ p, err := hostfiles.Open(ctx, data, prefix)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := p.Stage(ctx, "healthy.txt", []byte("fine"), 0o644, false); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := p.Commit(ctx); err != nil {
+ t.Fatal(err)
+ }
+ m, _, err := hostfiles.Load(ctx, data, prefix)
+ if err != nil {
+ t.Fatal(err)
+ }
+ m.Seq++
+ m.Entries[deep] = hostfiles.Entry{Seq: m.Seq, Deleted: true}
+ body, err := json.Marshal(m)
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, etag, err := data.Get(ctx, hostfiles.ManifestKey(prefix))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, err := data.Put(ctx, hostfiles.ManifestKey(prefix), body, etag); err != nil {
+ t.Fatal(err)
+ }
+
+ rctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+ defer cancel()
+ if _, st, err := hostfiles.Restore(rctx, data, prefix, dir, nil); err != nil || st.Removed != 1 {
+ t.Fatalf("Restore: %+v %v", st, err)
+ }
+ f := ReleaseFlushFiles{Data: data, Registry: reg, FilesDir: func(string) string { return dir }, EnvPath: func(h string) string { return filepath.Join(envDir, h+".env") }}
+ seq, err := f.Flush(ctx, hostclaim.Observed{Host: host, Claim: &hostclaim.Claim{Host: host, Holder: holder}})
+ if err != nil {
+ t.Fatalf("the release flush after the restore: %v", err)
+ }
+ got, _, err := hostfiles.Load(ctx, data, prefix)
+ if err != nil || got.Seq != seq || !got.Entries[deep].Deleted || got.Entries["healthy.txt"].Deleted {
+ t.Fatalf("after the flush: seq %d/%d, deep %+v, %v", got.Seq, seq, got.Entries[deep], err)
+ }
+}
diff --git a/internal/hostfiles/dir_unix.go b/internal/hostfiles/dir_unix.go
index 4148ab92d..ca0e91df0 100644
--- a/internal/hostfiles/dir_unix.go
+++ b/internal/hostfiles/dir_unix.go
@@ -3,6 +3,7 @@
package hostfiles
import (
+ "context"
"errors"
"io"
"os"
@@ -109,6 +110,13 @@ func notADirectory(err error) bool {
// reported as gone: publishOne relies on descend never leaving the tree.
// descend("") returns d itself, so the caller must not close it.
func (d *dirHandle) descend(rel string) (sub *dirHandle, gone bool, err error) {
+ return d.descendCtx(context.Background(), rel)
+}
+
+// descendCtx is descend, checking ctx before each component, so a walk
+// thousands of directories deep (Restore's removal of a tombstone past the
+// cap, removePastCap) stops with ctx's error when it is cancelled.
+func (d *dirHandle) descendCtx(ctx context.Context, rel string) (sub *dirHandle, gone bool, err error) {
if rel == "" {
return d, false, nil
}
@@ -120,6 +128,12 @@ func (d *dirHandle) descend(rel string) (sub *dirHandle, gone bool, err error) {
}
cur, owned := d, false
for _, part := range parts {
+ if err := ctx.Err(); err != nil {
+ if owned {
+ cur.Close()
+ }
+ return nil, false, err
+ }
betweenLookAndOpen()
next, err := cur.openDir(part)
if owned {
diff --git a/internal/hostfiles/fix5_linux_test.go b/internal/hostfiles/fix5_linux_test.go
index a6d414b52..8a9824237 100644
--- a/internal/hostfiles/fix5_linux_test.go
+++ b/internal/hostfiles/fix5_linux_test.go
@@ -10,6 +10,7 @@ import (
"os"
"path/filepath"
"slices"
+ "strconv"
"strings"
"testing"
"time"
@@ -221,19 +222,22 @@ func TestAScanRetiresAnInheritedSkippedEntryPastTheCap(t *testing.T) {
}
}
-// Astra's review of the exemption (P2 2): Restore never walks a tombstone
-// past the cap, so a deep tree standing at the restore's destination
-// costs it nothing and is left where it is (the next walk refuses it).
-func TestRestoreNeverWalksATombstonePastTheCap(t *testing.T) {
- ctx := context.Background()
- s, dir := release.NewDirStore(t.TempDir()), t.TempDir()
- // A file deeper than the cap, built one directory at a time.
+// deepLevels is how many directories deep the fixtures below go: the
+// parent is exactly MaxPathBytes ("d/d/.../d", 4095 bytes), so a walk may
+// enter it, and the file in it ("…/f", 4097 bytes) is past the cap.
+const deepLevels = MaxPathBytes/2 + 1
+
+// deepTree builds levels directories named "d" under dir, one at a time,
+// and returns the open root of the last one (the caller closes it) and
+// the path to it.
+func deepTree(t *testing.T, dir string, levels int) (*os.Root, []string) {
+ t.Helper()
r, err := os.OpenRoot(dir)
if err != nil {
t.Fatal(err)
}
var parts []string
- for n := 0; n < MaxPathBytes/2+1; n++ {
+ for n := 0; n < levels; n++ {
if err := r.Mkdir("d", 0o755); err != nil {
t.Fatal(err)
}
@@ -244,25 +248,91 @@ func TestRestoreNeverWalksATombstonePastTheCap(t *testing.T) {
}
r, parts = next, append(parts, "d")
}
+ return r, parts
+}
+
+// readDeep reads name beneath dir/parts..., one directory at a time, as
+// a path past PATH_MAX cannot be opened whole.
+func readDeep(dir string, parts []string, name string) ([]byte, error) {
+ r, err := os.OpenRoot(dir)
+ if err != nil {
+ return nil, err
+ }
+ for _, d := range parts {
+ next, err := r.OpenRoot(d)
+ r.Close()
+ if err != nil {
+ return nil, err
+ }
+ r = next
+ }
+ defer r.Close()
+ return r.ReadFile(name)
+}
+
+// deepTombstone builds the deep fixture at dir, a file "f" past the cap,
+// and inherits a manifest at pfx that tombstones it. It returns the
+// tombstoned path and its directories.
+func deepTombstone(t *testing.T, s release.Store, dir string) (string, []string) {
+ t.Helper()
+ r, parts := deepTree(t, dir, deepLevels)
if err := r.WriteFile("f", []byte("deep"), 0o600); err != nil {
t.Fatal(err)
}
r.Close()
deep := strings.Join(append(parts, "f"), "/")
- if withinTreeCap(deep) == nil {
- t.Fatalf("the fixture is within the cap: %d bytes", len(deep))
+ if withinTreeCap(deep) == nil || withinTreeCap(strings.Join(parts, "/")) != nil {
+ t.Fatalf("the fixture's file is within the cap, or its parent is not: %d bytes", len(deep))
}
inheritPastTheCap(t, s, func(seq int64) map[string]Entry {
return map[string]Entry{deep: {Seq: seq, Deleted: true}}
})
- _, st, err := Restore(ctx, s, pfx, dir, nil)
+ return deep, parts
+}
+
+// restoreWithin runs Restore and fails the test if it has not returned
+// within limit, rather than waiting for go test's timeout.
+func restoreWithin(t *testing.T, ctx context.Context, limit time.Duration, s release.Store, dir string) (RestoreStats, time.Duration, error) {
+ t.Helper()
+ type result struct {
+ st RestoreStats
+ err error
+ }
+ done, start := make(chan result, 1), time.Now()
+ go func() {
+ _, st, err := Restore(ctx, s, pfx, dir, nil)
+ done <- result{st, err}
+ }()
+ select {
+ case r := <-done:
+ return r.st, time.Since(start), r.err
+ case <-time.After(limit):
+ t.Fatalf("Restore was still running after %v", limit)
+ return RestoreStats{}, 0, nil
+ }
+}
+
+// Astra's review of the exemption, rounds 1 and 2: a tombstone past the
+// cap whose file still stands at the destination is removed, or the next
+// scan refuses the whole tree (ErrTooDeep) and nothing ever commits,
+// relocation's release flush among them. It is removed in one walk down
+// (removePastCap), linear in depth: removeInside's per-prefix walk is
+// cubic and takes minutes at this depth. Every directory stays, and
+// restore, scan and commit all go through.
+func TestRestoreRemovesATombstonePastTheCapInOneWalk(t *testing.T) {
+ s, dir := release.NewDirStore(t.TempDir()), t.TempDir()
+ deep, parts := deepTombstone(t, s, dir)
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ st, took, err := restoreWithin(t, ctx, 10*time.Second, s, dir)
if err != nil {
t.Fatalf("Restore: %v", err)
}
- if st.Removed != 0 || st.Written != 1 {
+ t.Logf("restore with a %d-byte tombstone %d directories deep: %v", len(deep), len(parts), took)
+ if st.Removed != 1 || st.Written != 1 {
t.Fatalf("restore: %+v", st)
}
- r, err = os.OpenRoot(dir)
+ r, err := os.OpenRoot(dir)
if err != nil {
t.Fatal(err)
}
@@ -270,12 +340,96 @@ func TestRestoreNeverWalksATombstonePastTheCap(t *testing.T) {
next, err := r.OpenRoot(d)
r.Close()
if err != nil {
- t.Fatalf("the deep tree was touched: %v", err)
+ t.Fatalf("a directory of the deep tree went: %v", err)
}
r = next
}
- defer r.Close()
- if b, err := r.ReadFile("f"); err != nil || string(b) != "deep" {
- t.Fatalf("the deep file: %q %v", b, err)
+ if _, err := r.Lstat("f"); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("the deep file is still there: %v", err)
+ }
+ r.Close()
+ p, err := Open(context.Background(), s, pfx)
+ if err != nil {
+ t.Fatalf("Open: %v", err)
+ }
+ if _, err := SyncDir(context.Background(), p, dir, nil); err != nil {
+ t.Fatalf("scan after the restore: %v", err)
+ }
+ commit(t, p)
+ m, _, err := Load(context.Background(), s, pfx)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !m.Entries[deep].Deleted || m.Entries["healthy.txt"].Deleted {
+ t.Fatalf("after the scan: deep %+v, healthy %+v", m.Entries[deep], m.Entries["healthy.txt"])
+ }
+}
+
+// The deep walk checks ctx between components: cancelled part way down,
+// Restore stops with ctx's error and removes nothing.
+func TestRestoreStopsADeepTombstoneWalkWhenCancelled(t *testing.T) {
+ s, dir := release.NewDirStore(t.TempDir()), t.TempDir()
+ _, parts := deepTombstone(t, s, dir)
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+ const at = 1000
+ calls := 0
+ orig := betweenLookAndOpen
+ t.Cleanup(func() { betweenLookAndOpen = orig })
+ betweenLookAndOpen = func() {
+ calls++
+ if calls == at {
+ cancel()
+ }
+ }
+ _, _, err := restoreWithin(t, ctx, 10*time.Second, s, dir)
+ if !errors.Is(err, context.Canceled) {
+ t.Fatalf("Restore with ctx cancelled part way down: %v", err)
+ }
+ if calls != at {
+ t.Fatalf("the walk went on after the cancel: %d components opened, cancelled at %d", calls, at)
+ }
+ if b, err := readDeep(dir, parts, "f"); err != nil || string(b) != "deep" {
+ t.Fatalf("a cancelled restore removed the deep file: %v", err)
+ }
+}
+
+// The deep walk never follows a symlink: one standing for a directory
+// anywhere on the way (the first, the middle, the parent) is "nothing of
+// ours here", and the file it points at, outside the root, stays.
+func TestRestoreNeverFollowsASymlinkOnADeepTombstone(t *testing.T) {
+ for _, k := range []int{0, deepLevels / 2, deepLevels - 1} {
+ t.Run(strconv.Itoa(k), func(t *testing.T) {
+ s, dir, outside := release.NewDirStore(t.TempDir()), t.TempDir(), t.TempDir()
+ // The real file the tombstone names, if the link were followed:
+ // the rest of the path, built outside the root.
+ r, rest := deepTree(t, outside, deepLevels-k-1)
+ if err := r.WriteFile("f", []byte("outside"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ r.Close()
+ r, parts := deepTree(t, dir, k)
+ if err := r.Symlink(outside, "d"); err != nil {
+ t.Fatal(err)
+ }
+ r.Close()
+ deep := strings.Join(append(append(append(parts, "d"), rest...), "f"), "/")
+ if strings.Count(deep, "/") != deepLevels || withinTreeCap(deep) == nil {
+ t.Fatalf("the fixture is not the deep path: %d bytes", len(deep))
+ }
+ inheritPastTheCap(t, s, func(seq int64) map[string]Entry {
+ return map[string]Entry{deep: {Seq: seq, Deleted: true}}
+ })
+ st, _, err := restoreWithin(t, context.Background(), 10*time.Second, s, dir)
+ if err != nil {
+ t.Fatalf("Restore: %v", err)
+ }
+ if st.Removed != 0 {
+ t.Fatalf("restore through a symlink: %+v", st)
+ }
+ if b, err := readDeep(outside, rest, "f"); err != nil || string(b) != "outside" {
+ t.Fatalf("the file outside the root: %q %v", b, err)
+ }
+ })
}
}
diff --git a/internal/hostfiles/restore.go b/internal/hostfiles/restore.go
index 9dcb18d3c..dc66c05c8 100644
--- a/internal/hostfiles/restore.go
+++ b/internal/hostfiles/restore.go
@@ -106,16 +106,9 @@ func restore(ctx context.Context, s release.Store, prefix string, origins []stri
}
var live, dead []string
for p, e := range m.Entries {
- switch {
- case e.Deleted && withinTreeCap(p) != nil:
- // A tombstone past the cap (one a manifest written before the cap
- // may hold) is never walked: removing it would resolve every
- // component of a path no restore could have written, without
- // bound. Anything still standing there is left to the next walk,
- // which refuses a path past the cap (ErrTooDeep).
- case e.Deleted:
+ if e.Deleted {
dead = append(dead, p)
- default:
+ } else {
live = append(live, p)
}
}
@@ -149,8 +142,19 @@ func restore(ctx context.Context, s release.Store, prefix string, origins []stri
return dead[i] < dead[j]
})
for _, p := range dead {
- mark(p)
- removed, err := removeInside(root, p)
+ var removed bool
+ if withinTreeCap(p) != nil {
+ // A tombstone past the cap (one a manifest written before the
+ // cap may hold) is removed in one linear, cancellable walk that
+ // syncs its own parent, and is never marked: removeInside and
+ // the directory sync below resolve every prefix afresh, which
+ // is cubic in depth. Left standing, the file would make the
+ // next walk refuse the whole tree (ErrTooDeep).
+ removed, err = removePastCap(ctx, root, p)
+ } else {
+ mark(p)
+ removed, err = removeInside(root, p)
+ }
if err != nil {
return Manifest{}, st, fmt.Errorf("hostfiles: remove %s: %w", p, err)
}
diff --git a/internal/hostfiles/restore_other.go b/internal/hostfiles/restore_other.go
new file mode 100644
index 000000000..4670f3e07
--- /dev/null
+++ b/internal/hostfiles/restore_other.go
@@ -0,0 +1,17 @@
+//go:build !unix
+
+package hostfiles
+
+import (
+ "context"
+ "errors"
+ "os"
+)
+
+// removePastCap needs openat and unlinkat to remove a path past the cap in
+// one walk (restore_unix.go). The agent runs on Linux; elsewhere a
+// tombstone past the cap is refused rather than removed by a walk without
+// bound.
+func removePastCap(ctx context.Context, root *os.Root, rel string) (bool, error) {
+ return false, errors.New("hostfiles: removing a tombstone past the path cap is not supported on this OS")
+}
diff --git a/internal/hostfiles/restore_unix.go b/internal/hostfiles/restore_unix.go
new file mode 100644
index 000000000..5bff6bd08
--- /dev/null
+++ b/internal/hostfiles/restore_unix.go
@@ -0,0 +1,80 @@
+//go:build unix
+
+package hostfiles
+
+import (
+ "context"
+ "errors"
+ "os"
+ "path"
+ "strings"
+
+ "golang.org/x/sys/unix"
+)
+
+// removePastCap is removeInside for a tombstone past MaxPathBytes, one a
+// manifest written before the cap may hold. removeInside resolves every
+// prefix of rel afresh (realParents, then os.Root), which is cubic in
+// depth; this walks down once instead: each directory is opened beneath
+// the one before it with O_NOFOLLOW (descendCtx), so it is O(depth)
+// syscalls, no symlink is ever followed and nothing outside root is
+// reached, and ctx is checked between components.
+//
+// The semantics are removeInside's. A missing component, or one that is a
+// symlink or anything but a directory, means nothing of ours is there: not
+// an error, nothing removed. So is a missing final name. A final name that
+// is a directory is left alone.
+//
+// Durability: Restore's directory sync re-resolves every touched directory
+// from the root, which is the same cubic walk, so a path past the cap is
+// never marked. Instead the parent, once reached, is fsynced here through
+// the descriptor already held, whether or not this call removed anything
+// (an earlier attempt may have removed the name and failed before its
+// sync). Only the parent's entries change, so when this returns nil the
+// removal is durable.
+func removePastCap(ctx context.Context, root *os.Root, rel string) (bool, error) {
+ f, err := root.Open(".")
+ if err != nil {
+ return false, err
+ }
+ defer f.Close()
+ top, err := handleOf(f)
+ if err != nil {
+ return false, err
+ }
+ parent, name := path.Split(rel)
+ dir, gone, err := top.descendCtx(ctx, strings.TrimSuffix(parent, "/"))
+ if err != nil || gone {
+ return false, err
+ }
+ if dir != top {
+ defer dir.Close()
+ }
+ if err := ctx.Err(); err != nil {
+ return false, err
+ }
+ removed := false
+ st, err := dir.look(name)
+ switch {
+ case errors.Is(err, unix.ENOENT):
+ case err != nil:
+ return false, &os.PathError{Op: "fstatat", Path: name, Err: err}
+ case isDir(st):
+ // A directory now: a later entry lives under it, and the tombstone
+ // is for a file that no longer exists.
+ default:
+ err := unix.Unlinkat(int(dir.f.Fd()), name, 0)
+ switch {
+ case err == nil:
+ removed = true
+ case errors.Is(err, unix.ENOENT), errors.Is(err, unix.EISDIR):
+ // Gone, or swapped for a directory, since the look.
+ default:
+ return false, &os.PathError{Op: "unlinkat", Path: name, Err: err}
+ }
+ }
+ if err := dir.f.Sync(); err != nil {
+ return false, err
+ }
+ return removed, nil
+}