amadan

carlos / platform Public

hostfiles: Publish refuses a pass with a replaced or torn file, CopyManifest refuses a source not at the expected seq, and Restore and ReadPlatform refuse a malformed prefix

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude 3283c7881363818bc9762ddd6c5004e034f2ce3d parent 7cff1e7
5 files changed, +196 −18
  • internal/hostfiles/contract.go +37 −7
  • internal/hostfiles/origins_test.go +60 −11
  • internal/hostfiles/restore.go +6 −0
  • internal/hostfiles/sync.go +11 −0
  • internal/hostfiles/sync_unix_test.go +82 −0
diff --git a/internal/hostfiles/contract.go b/internal/hostfiles/contract.go
index 7f142e08d..b08b2fcd5 100644
--- a/internal/hostfiles/contract.go
+++ b/internal/hostfiles/contract.go
@@ -19,6 +19,10 @@ import (
// manifest. The copy wrote nothing.
var ErrManifestExists = errors.New("hostfiles: the target generation already has a manifest")
+// ErrSourceMoved means CopyManifest's source is not at the Seq the caller
+// expected. The copy wrote nothing.
+var ErrSourceMoved = errors.New("hostfiles: the source manifest is not at the expected seq")
+
// CopyManifest establishes toPrefix's manifest as an exact copy of
// fromPrefix's: the same sequence number and the same entries, each still
// naming its object by its origin-qualified key. No object is copied; the
@@ -31,10 +35,20 @@ var ErrManifestExists = errors.New("hostfiles: the target generation already has
// its pin admits. Every entry is validated before anything is written; an
// absent source is os.ErrNotExist, never an empty copy.
//
-// The write is create-only. A target that already holds exactly these
-// bytes (a repeat whose first response was lost) is success; any other
-// target is ErrManifestExists and is left as it was.
-func CopyManifest(ctx context.Context, s release.Store, fromPrefix, toPrefix string, origins []string) (Manifest, error) {
+// expectSeq, when not 0, is the source revision the caller admitted. A
+// source at any other Seq is ErrSourceMoved and nothing is written, on a
+// first attempt or a retry. 0 copies whatever the source holds.
+//
+// The write is create-only. A target that already holds exactly the
+// manifest this call would write (a repeat whose first response was lost)
+// is success; any other target is ErrManifestExists and is left as it was.
+// That retry-safety holds only while the source cannot commit between
+// attempts: the source generation must be fenced or frozen, as the claim's
+// admission makes it. Against a live source a retry may find the target
+// holding an older revision and report ErrManifestExists; with expectSeq
+// set it reports ErrSourceMoved instead, and never copies a revision the
+// caller did not admit.
+func CopyManifest(ctx context.Context, s release.Store, fromPrefix, toPrefix string, expectSeq int64, origins []string) (Manifest, error) {
if err := checkPrefix(fromPrefix); err != nil {
return Manifest{}, err
}
@@ -55,6 +69,9 @@ func CopyManifest(ctx context.Context, s release.Store, fromPrefix, toPrefix str
if !found {
return Manifest{}, fmt.Errorf("hostfiles: copy from %s: %w", fromPrefix, os.ErrNotExist)
}
+ if expectSeq != 0 && m.Seq != expectSeq {
+ return Manifest{}, fmt.Errorf("%w: %s is at seq %d, not %d", ErrSourceMoved, fromPrefix, m.Seq, expectSeq)
+ }
if err := checkManifest(m, origins); err != nil {
return Manifest{}, err
}
@@ -85,9 +102,16 @@ func CopyManifest(ctx context.Context, s release.Store, fromPrefix, toPrefix str
// everything under it. On success every regular file in dir that skip does
// not exclude is in the manifest: published, or recorded as skipped when
// it is larger than MaxFileBytes (st.TooLarge). Anything that is not a
-// regular file is left out and listed in st.NotRegular. A file that
-// exists but cannot be read is an error, and nothing is committed: the
-// manifest would otherwise not hold it.
+// regular file is left out and listed in st.NotRegular.
+//
+// Publish is an install-time contract, so it commits a complete and
+// consistent image or nothing. It returns an error, and commits nothing,
+// when any file could not be read (st.Unreadable), was replaced between
+// the look and the open (st.Retry), or changed while it was read
+// (st.Torn). The manifest would otherwise lack the file or hold bytes that
+// were never the file's state. The caller stops writers and calls Publish
+// again. Objects uploaded by a refused attempt are referenced by no
+// manifest.
//
// A prefix that already has a manifest is continued, not replaced: a
// repeat with nothing changed returns the same sequence number. Another
@@ -107,6 +131,12 @@ func Publish(ctx context.Context, s release.Store, prefix, dir string, skip func
if len(st.Unreadable) > 0 {
return 0, st, fmt.Errorf("hostfiles: publish %s: %d file(s) could not be read: %q", dir, len(st.Unreadable), st.Unreadable)
}
+ if len(st.Retry) > 0 {
+ return 0, st, fmt.Errorf("hostfiles: publish %s: %d file(s) were replaced while being looked at: %q", dir, len(st.Retry), st.Retry)
+ }
+ if len(st.Torn) > 0 {
+ return 0, st, fmt.Errorf("hostfiles: publish %s: %d file(s) changed while being read: %q", dir, len(st.Torn), st.Torn)
+ }
seq, err := p.Commit(ctx)
return seq, st, err
}
diff --git a/internal/hostfiles/origins_test.go b/internal/hostfiles/origins_test.go
index f9e0d1653..95efbecab 100644
--- a/internal/hostfiles/origins_test.go
+++ b/internal/hostfiles/origins_test.go
@@ -61,7 +61,7 @@ func TestEntriesCarryTheirOriginQualifiedObjectKey(t *testing.T) {
func TestACopiedManifestRestoresTheSameBytesWithoutCopyingObjects(t *testing.T) {
ctx, s := context.Background(), release.NewDirStore(t.TempDir())
publishG1(t, s)
- m, err := CopyManifest(ctx, s, pfx, pfx2, nil)
+ m, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil)
if err != nil {
t.Fatal(err)
}
@@ -115,7 +115,7 @@ func TestACopiedManifestRestoresTheSameBytesWithoutCopyingObjects(t *testing.T)
func TestRestoreRefusesAnEntryOutsideTheAllowedOriginsAndTouchesNothing(t *testing.T) {
ctx, s := context.Background(), release.NewDirStore(t.TempDir())
publishG1(t, s)
- m, err := CopyManifest(ctx, s, pfx, pfx2, nil)
+ m, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil)
if err != nil {
t.Fatal(err)
}
@@ -209,7 +209,7 @@ func TestAManifestOfAnotherVersionIsRefused(t *testing.T) {
if _, _, err := Restore(ctx, s, pfx, t.TempDir(), nil); err == nil {
t.Fatalf("v%d: Restore accepted it", v)
}
- if _, err := CopyManifest(ctx, s, pfx, pfx2, nil); err == nil {
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil); err == nil {
t.Fatalf("v%d: CopyManifest accepted it", v)
}
}
@@ -218,11 +218,11 @@ func TestAManifestOfAnotherVersionIsRefused(t *testing.T) {
func TestCopyManifestIsCreateOnlyAndRetrySafe(t *testing.T) {
ctx, s := context.Background(), release.NewDirStore(t.TempDir())
publishG1(t, s)
- if _, err := CopyManifest(ctx, s, pfx, pfx2, nil); err != nil {
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil); err != nil {
t.Fatal(err)
}
// A repeat whose first response was lost finds its own bytes: success.
- if _, err := CopyManifest(ctx, s, pfx, pfx2, nil); err != nil {
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil); err != nil {
t.Fatalf("a repeated copy: %v", err)
}
// Once g2 has moved on, a copy is refused and changes nothing.
@@ -233,7 +233,7 @@ func TestCopyManifestIsCreateOnlyAndRetrySafe(t *testing.T) {
stage(t, p, "new", "n")
commit(t, p)
before, _, _ := s.Get(ctx, ManifestKey(pfx2))
- if _, err := CopyManifest(ctx, s, pfx, pfx2, nil); !errors.Is(err, ErrManifestExists) {
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil); !errors.Is(err, ErrManifestExists) {
t.Fatalf("err = %v, want ErrManifestExists", err)
}
after, _, _ := s.Get(ctx, ManifestKey(pfx2))
@@ -244,27 +244,27 @@ func TestCopyManifestIsCreateOnlyAndRetrySafe(t *testing.T) {
func TestCopyManifestRefusesAMissingOrInvalidSourceAndWritesNothing(t *testing.T) {
ctx, s := context.Background(), release.NewDirStore(t.TempDir())
- if _, err := CopyManifest(ctx, s, pfx, pfx2, nil); !errors.Is(err, os.ErrNotExist) {
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil); !errors.Is(err, os.ErrNotExist) {
t.Fatalf("absent source: err = %v, want os.ErrNotExist", err)
}
// g2 copied from g1 references g1; copying g2 on to g3 with only g2
// admitted is refused.
publishG1(t, s)
- if _, err := CopyManifest(ctx, s, pfx, pfx2, nil); err != nil {
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, 0, nil); err != nil {
t.Fatal(err)
}
const pfx3 = "instances/shop.test/abc/files/g3/"
- if _, err := CopyManifest(ctx, s, pfx2, pfx3, nil); err == nil || !strings.Contains(err.Error(), "origin") {
+ if _, err := CopyManifest(ctx, s, pfx2, pfx3, 0, nil); err == nil || !strings.Contains(err.Error(), "origin") {
t.Fatalf("err = %v, want an origin refusal", err)
}
if _, _, err := s.Get(ctx, ManifestKey(pfx3)); !errors.Is(err, os.ErrNotExist) {
t.Fatalf("a refused copy wrote a manifest: %v", err)
}
- if _, err := CopyManifest(ctx, s, pfx2, pfx3, []string{pfx, pfx2}); err != nil {
+ if _, err := CopyManifest(ctx, s, pfx2, pfx3, 0, []string{pfx, pfx2}); err != nil {
t.Fatalf("with g1 admitted: %v", err)
}
for _, bad := range [][2]string{{pfx, pfx}, {pfx, "instances/shop.test/abc/files/g4"}, {"nope", pfx2}} {
- if _, err := CopyManifest(ctx, s, bad[0], bad[1], nil); err == nil {
+ if _, err := CopyManifest(ctx, s, bad[0], bad[1], 0, nil); err == nil {
t.Fatalf("CopyManifest(%q, %q) accepted", bad[0], bad[1])
}
}
@@ -302,3 +302,52 @@ func TestPublishCommitsTheDirectoryAsOneRevision(t *testing.T) {
t.Fatal("published a missing directory")
}
}
+
+// expectSeq pins the copy to the source revision the caller admitted: a
+// source that has moved on is refused, on a first attempt or a retry, and
+// nothing is written.
+func TestCopyManifestRefusesASourceThatIsNotAtTheExpectedSeq(t *testing.T) {
+ ctx, s := context.Background(), release.NewDirStore(t.TempDir())
+ publishG1(t, s)
+ src, _, _ := Load(ctx, s, pfx)
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, src.Seq+1, nil); !errors.Is(err, ErrSourceMoved) {
+ t.Fatalf("err = %v, want ErrSourceMoved", err)
+ }
+ if _, _, err := s.Get(ctx, ManifestKey(pfx2)); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("a refused copy wrote a manifest: %v", err)
+ }
+ m, err := CopyManifest(ctx, s, pfx, pfx2, src.Seq, nil)
+ if err != nil || m.Seq != src.Seq {
+ t.Fatalf("m.Seq = %d, err = %v", m.Seq, err)
+ }
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, src.Seq, nil); err != nil {
+ t.Fatalf("a repeat at the same seq: %v", err)
+ }
+ // The source commits again: a retry pinned to the old seq is refused,
+ // and the target keeps the pinned copy.
+ p := open(t, s)
+ stage(t, p, "later", "x")
+ commit(t, p)
+ before, _, _ := s.Get(ctx, ManifestKey(pfx2))
+ if _, err := CopyManifest(ctx, s, pfx, pfx2, src.Seq, nil); !errors.Is(err, ErrSourceMoved) {
+ t.Fatalf("err = %v, want ErrSourceMoved", err)
+ }
+ if after, _, _ := s.Get(ctx, ManifestKey(pfx2)); string(before) != string(after) {
+ t.Fatal("a refused copy changed the target")
+ }
+}
+
+func TestRestoreAndReadPlatformRefuseAMalformedPrefix(t *testing.T) {
+ ctx, s := context.Background(), release.NewDirStore(t.TempDir())
+ publishG1(t, s)
+ m, _, _ := Load(ctx, s, pfx)
+ for _, bad := range []string{"", "instances/shop.test/abc/files/g1", "/" + pfx, "instances/../" + pfx, "instances//x/"} {
+ // Admitting pfx explicitly means only the prefix check can refuse.
+ if _, _, err := Restore(ctx, s, bad, t.TempDir(), []string{pfx}); err == nil {
+ t.Fatalf("Restore accepted prefix %q", bad)
+ }
+ if _, err := ReadPlatform(ctx, s, bad, m, "env", []string{pfx}); err == nil {
+ t.Fatalf("ReadPlatform accepted prefix %q", bad)
+ }
+ }
+}
diff --git a/internal/hostfiles/restore.go b/internal/hostfiles/restore.go
index 736b59260..523f2ee1d 100644
--- a/internal/hostfiles/restore.go
+++ b/internal/hostfiles/restore.go
@@ -58,6 +58,9 @@ type RestoreStats struct {
// well formed but its origin is not admitted).
func Restore(ctx context.Context, s release.Store, prefix, dir string, origins []string) (Manifest, RestoreStats, error) {
var st RestoreStats
+ if err := checkPrefix(prefix); err != nil {
+ return Manifest{}, st, err
+ }
origins, err := admitted(prefix, origins)
if err != nil {
return Manifest{}, st, err
@@ -198,6 +201,9 @@ func Restore(ctx context.Context, s release.Store, prefix, dir string, origins [
// the manifest does not hold is os.ErrNotExist. prefix is where m was
// loaded from; origins are as for Restore (nil or empty: prefix alone).
func ReadPlatform(ctx context.Context, s release.Store, prefix string, m Manifest, name string, origins []string) ([]byte, error) {
+ if err := checkPrefix(prefix); err != nil {
+ return nil, err
+ }
origins, err := admitted(prefix, origins)
if err != nil {
return nil, err
diff --git a/internal/hostfiles/sync.go b/internal/hostfiles/sync.go
index 8604a1e1f..474d3e24d 100644
--- a/internal/hostfiles/sync.go
+++ b/internal/hostfiles/sync.go
@@ -27,9 +27,14 @@ var (
readAll = io.ReadAll
)
+// SyncStats reports one pass. Retry lists regular files replaced between
+// the look and the open: nothing was read and nothing staged for them this
+// pass. Torn lists files staged from a read during which the file changed,
+// so their staged bytes may be an inconsistent image.
type SyncStats struct {
Published, Deleted int
TooLarge, NotRegular, Unreadable []string
+ Retry, Torn []string
}
// skipped reports whether rel, or any directory above it, is excluded.
@@ -90,6 +95,7 @@ func syncRoot(ctx context.Context, p *Publisher, root *dirHandle, skip func(rel
return nil
case fileRetry:
seen[rel] = true // replaced mid-look: the next scan reads it
+ st.Retry = append(st.Retry, rel)
return nil
case fileTooLarge:
seen[rel] = true
@@ -104,6 +110,9 @@ func syncRoot(ctx context.Context, p *Publisher, root *dirHandle, skip func(rel
if p.pending() != before {
st.Published++
}
+ if state == fileTorn {
+ st.Torn = append(st.Torn, rel)
+ }
return nil
})
if err != nil {
@@ -121,5 +130,7 @@ func syncRoot(ctx context.Context, p *Publisher, root *dirHandle, skip func(rel
sort.Strings(st.TooLarge)
sort.Strings(st.NotRegular)
sort.Strings(st.Unreadable)
+ sort.Strings(st.Retry)
+ sort.Strings(st.Torn)
return st, nil
}
diff --git a/internal/hostfiles/sync_unix_test.go b/internal/hostfiles/sync_unix_test.go
index c811cb715..33006cc32 100644
--- a/internal/hostfiles/sync_unix_test.go
+++ b/internal/hostfiles/sync_unix_test.go
@@ -4,6 +4,7 @@ package hostfiles
import (
"context"
+ "io"
"net"
"os"
"path/filepath"
@@ -272,3 +273,84 @@ func TestPublishRefusesADirectoryWithAnUnreadableFile(t *testing.T) {
t.Fatalf("a manifest was committed: found = %v, err = %v", found, err)
}
}
+
+// replaceAt swaps src/f for new content at the n-th look-to-open window.
+func replaceAt(t *testing.T, n int, src string) {
+ t.Helper()
+ swapAt(t, n, func() {
+ tmp := filepath.Join(src, "f.new")
+ if err := os.WriteFile(tmp, []byte("v2"), 0o640); err != nil {
+ t.Error(err)
+ }
+ if err := os.Rename(tmp, filepath.Join(src, "f")); err != nil {
+ t.Error(err)
+ }
+ })
+}
+
+// A file replaced mid-look is reported, not silently left out.
+func TestSyncDirReportsAFileReplacedMidLook(t *testing.T) {
+ ctx, s, src := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir()
+ write(t, src, "f", "v1")
+ replaceAt(t, 1, src)
+ st, err := SyncDir(ctx, open(t, s), src, nil)
+ if err != nil || !reflect.DeepEqual(st.Retry, []string{"f"}) {
+ t.Fatalf("stats = %+v err = %v, want Retry [f]", st, err)
+ }
+}
+
+// A first install whose only file was replaced mid-look would commit a
+// manifest without it.
+func TestPublishRefusesADirectoryWithAFileReplacedMidLook(t *testing.T) {
+ ctx, s, src := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir()
+ write(t, src, "ok", "v1")
+ write(t, src, "f", "v1")
+ replaceAt(t, 1, src) // "f" sorts before "ok"
+ if _, st, err := Publish(ctx, s, pfx, src, nil); err == nil || !reflect.DeepEqual(st.Retry, []string{"f"}) {
+ t.Fatalf("err = %v, stats = %+v", err, st)
+ }
+ if _, found, err := Load(ctx, s, pfx); found || err != nil {
+ t.Fatalf("a manifest was committed: found = %v, err = %v", found, err)
+ }
+}
+
+// tearAt makes the read of src/f see the file grow while it is read.
+func tearAt(t *testing.T, src string) {
+ t.Helper()
+ orig := readAll
+ t.Cleanup(func() { readAll = orig })
+ readAll = func(r io.Reader) ([]byte, error) {
+ b, err := orig(r)
+ f, oerr := os.OpenFile(filepath.Join(src, "f"), os.O_APPEND|os.O_WRONLY, 0)
+ if oerr != nil {
+ t.Error(oerr)
+ return b, err
+ }
+ f.Write([]byte("more"))
+ f.Close()
+ return b, err
+ }
+}
+
+func TestSyncDirReportsATornRead(t *testing.T) {
+ ctx, s, src := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir()
+ write(t, src, "f", "v1")
+ tearAt(t, src)
+ st, err := SyncDir(ctx, open(t, s), src, nil)
+ if err != nil || !reflect.DeepEqual(st.Torn, []string{"f"}) {
+ t.Fatalf("stats = %+v err = %v, want Torn [f]", st, err)
+ }
+}
+
+// Publish is an install-time record: a torn image is not committed.
+func TestPublishRefusesATornRead(t *testing.T) {
+ ctx, s, src := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir()
+ write(t, src, "f", "v1")
+ tearAt(t, src)
+ if _, st, err := Publish(ctx, s, pfx, src, nil); err == nil || !reflect.DeepEqual(st.Torn, []string{"f"}) {
+ t.Fatalf("err = %v, stats = %+v", err, st)
+ }
+ if _, found, err := Load(ctx, s, pfx); found || err != nil {
+ t.Fatalf("a manifest was committed: found = %v, err = %v", found, err)
+ }
+}