amadan
hostfiles: a lost commit response is not a supersede, restore masks modes and resolves overlapping entries
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
3 files changed,
+137
−6
internal/hostfiles/hostfiles_test.go+93 −0internal/hostfiles/publisher.go+9 −1internal/hostfiles/restore.go+35 −5
diff --git a/internal/hostfiles/hostfiles_test.go b/internal/hostfiles/hostfiles_test.go| index ca603ac23..757e5eda6 100644 |
| --- a/internal/hostfiles/hostfiles_test.go |
| +++ b/internal/hostfiles/hostfiles_test.go |
| @@ -2,6 +2,7 @@ package hostfiles |
| import ( |
| "context" |
| + "encoding/json" |
| "errors" |
| "os" |
| "path/filepath" |
| @@ -487,3 +488,95 @@ func TestRestoreHandlesADeepDirectoryBecomingAFile(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// A store whose manifest Put is applied and then reports a lost response, once. |
| +type lostResponseStore struct { |
| + release.Store |
| + armed bool |
| +} |
| + |
| +func (l *lostResponseStore) Put(ctx context.Context, key string, body []byte, etag string) (string, error) { |
| + got, err := l.Store.Put(ctx, key, body, etag) |
| + if err == nil && l.armed && strings.HasSuffix(key, "manifest.json") { |
| + l.armed = false |
| + return "", errors.New("connection reset") |
| + } |
| + return got, err |
| +} |
| + |
| +func TestALostCommitResponseIsNotASupersede(t *testing.T) { |
| + ctx := context.Background() |
| + s := &lostResponseStore{Store: release.NewDirStore(t.TempDir())} |
| + p := open(t, s) |
| + stage(t, p, "x", "1") |
| + s.armed = true |
| + if _, err := p.Commit(ctx); err == nil || errors.Is(err, ErrSuperseded) { |
| + t.Fatalf("first commit err = %v, want the transport error", err) |
| + } |
| + seq, err := p.Commit(ctx) |
| + if err != nil || seq != 1 { |
| + t.Fatalf("second commit = %d, %v", seq, err) |
| + } |
| + stage(t, p, "y", "2") |
| + if seq := commit(t, p); seq != 2 { |
| + t.Fatalf("seq = %d, want 2", seq) |
| + } |
| +} |
| + |
| +func TestRestoreMasksTheModeToPermissionBits(t *testing.T) { |
| + ctx, s := context.Background(), release.NewDirStore(t.TempDir()) |
| + p := open(t, s) |
| + stage(t, p, "x", "x") |
| + commit(t, p) |
| + m, etag, _, err := load(ctx, s, pfx) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + e := m.Entries["x"] |
| + e.Mode = 0o4755 |
| + m.Entries["x"] = e |
| + body, _ := json.Marshal(m) |
| + if _, err := s.Put(ctx, ManifestKey(pfx), body, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + dir := t.TempDir() |
| + if _, _, err := Restore(ctx, s, pfx, dir); err != nil { |
| + t.Fatal(err) |
| + } |
| + fi, err := os.Stat(filepath.Join(dir, "x")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if fi.Mode() != 0o755 { |
| + t.Fatalf("mode = %v, want -rwxr-xr-x with no special bits", fi.Mode()) |
| + } |
| +} |
| + |
| +func TestOverlappingLiveEntriesKeepTheNewerOne(t *testing.T) { |
| + ctx, s := context.Background(), release.NewDirStore(t.TempDir()) |
| + p := open(t, s) |
| + stage(t, p, "a/b", "child") |
| + stage(t, p, "a", "file") // newer than the child |
| + commit(t, p) |
| + dir := t.TempDir() |
| + _, st, err := Restore(ctx, s, pfx, dir) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got := read(t, dir, "a"); got != "file" || !reflect.DeepEqual(st.Superseded, []string{"a/b"}) || st.Written != 1 { |
| + t.Fatalf("a = %q, superseded %v, written %d", got, st.Superseded, st.Written) |
| + } |
| + |
| + q := open(t, release.NewDirStore(t.TempDir())) |
| + stage(t, q, "a", "file") |
| + stage(t, q, "a/b", "child") // newer than the file |
| + commit(t, q) |
| + dir2 := t.TempDir() |
| + _, st, err = Restore(ctx, q.s, pfx, dir2) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got := read(t, dir2, "a/b"); got != "child" || !reflect.DeepEqual(st.Superseded, []string{"a"}) || st.Written != 1 { |
| + t.Fatalf("a/b = %q, superseded %v, written %d", got, st.Superseded, st.Written) |
| + } |
| +} |
diff --git a/internal/hostfiles/publisher.go b/internal/hostfiles/publisher.go| index 2d9880b89..be508a715 100644 |
| --- a/internal/hostfiles/publisher.go |
| +++ b/internal/hostfiles/publisher.go |
| @@ -1,6 +1,7 @@ |
| package hostfiles |
| import ( |
| + "bytes" |
| "context" |
| "crypto/sha256" |
| "encoding/hex" |
| @@ -156,7 +157,14 @@ func (p *Publisher) Commit(ctx context.Context) (int64, error) { |
| } |
| etag, err := p.s.Put(ctx, ManifestKey(p.prefix), body, p.etag) |
| if errors.Is(err, release.ErrCASConflict) { |
| - return 0, fmt.Errorf("%w: %s", ErrSuperseded, p.prefix) |
| + // A lost response looks like this: the store applied our earlier Put, |
| + // the reply never arrived, and this retry's etag is now stale. If the |
| + // manifest holds exactly the bytes we were writing, it is ours. |
| + cur, curEtag, gerr := p.s.Get(ctx, ManifestKey(p.prefix)) |
| + if gerr != nil || !bytes.Equal(cur, body) { |
| + return 0, fmt.Errorf("%w: %s", ErrSuperseded, p.prefix) |
| + } |
| + err, etag = nil, curEtag |
| } |
| if err != nil { |
| return 0, err |
diff --git a/internal/hostfiles/restore.go b/internal/hostfiles/restore.go| index 37f44fb41..5582687ad 100644 |
| --- a/internal/hostfiles/restore.go |
| +++ b/internal/hostfiles/restore.go |
| @@ -29,12 +29,15 @@ var syncDir = func(root *os.Root, rel string) error { |
| type RestoreStats struct { |
| Written, Removed int |
| Skipped []string // paths the manifest holds as present but unpublished |
| + Superseded []string // live paths not written: a live entry above or below them is newer |
| } |
| -// Restore makes dir match the manifest under prefix: it removes every |
| -// tombstoned entry, writes every published one, and reports the skipped |
| -// ones. It reads nothing but the manifest and the objects the manifest |
| -// names, and verifies each object against the hash the manifest records. |
| +// Restore writes into dir what the manifest under prefix names and removes |
| +// what it tombstones: every tombstoned entry is removed, every published one |
| +// written, and the skipped ones reported. It does not delete files the |
| +// manifest does not name. It reads nothing but the manifest and the objects |
| +// the manifest names, and verifies each object against the hash the manifest |
| +// records. |
| // |
| // Every path is validated before anything is touched, so a manifest with |
| // one bad path changes nothing. Every file operation goes through an |
| @@ -102,7 +105,34 @@ func Restore(ctx context.Context, s release.Store, prefix, dir string) (Manifest |
| } |
| } |
| sort.Strings(live) |
| + // A live file and a live entry beneath it cannot both exist. The newer |
| + // one (higher Seq) wins; the other is superseded and left alone. |
| + isLive := make(map[string]bool, len(live)) |
| for _, p := range live { |
| + isLive[p] = true |
| + } |
| + loser := map[string]bool{} |
| + for _, p := range live { |
| + for a := path.Dir(p); a != "."; a = path.Dir(a) { |
| + if !isLive[a] { |
| + continue |
| + } |
| + if m.Entries[a].Seq > m.Entries[p].Seq { |
| + loser[p] = true |
| + } else { |
| + loser[a] = true |
| + } |
| + } |
| + } |
| + for _, p := range live { |
| + if loser[p] { |
| + st.Superseded = append(st.Superseded, p) |
| + } |
| + } |
| + for _, p := range live { |
| + if loser[p] { |
| + continue |
| + } |
| e := m.Entries[p] |
| if e.Skipped != "" { |
| st.Skipped = append(st.Skipped, p) |
| @@ -113,7 +143,7 @@ func Restore(ctx context.Context, s release.Store, prefix, dir string) (Manifest |
| if err != nil { |
| return Manifest{}, st, fmt.Errorf("hostfiles: %s: %w", p, err) |
| } |
| - if err := writeInside(root, p, body, os.FileMode(e.Mode)); err != nil { |
| + if err := writeInside(root, p, body, os.FileMode(e.Mode&0o777)); err != nil { |
| return Manifest{}, st, fmt.Errorf("hostfiles: write %s: %w", p, err) |
| } |
| st.Written++ |