plan: region HA 1a rev 2 in progress, host-files code replaced by a tested dry run
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+926
−192
docs/superpowers/plans/2026-10-05-region-ha-1a-foundations.md+926 −192
diff --git a/docs/superpowers/plans/2026-10-05-region-ha-1a-foundations.md b/docs/superpowers/plans/2026-10-05-region-ha-1a-foundations.md| index 80a6a874a..3321aed1e 100644 |
| --- a/docs/superpowers/plans/2026-10-05-region-ha-1a-foundations.md |
| +++ b/docs/superpowers/plans/2026-10-05-region-ha-1a-foundations.md |
| @@ -252,10 +252,10 @@ func TestLegacyLabelNeedsABindNamingThisExactRecord(t *testing.T) { |
| t.Fatalf("good bind: %v", err) |
| } |
| for name, b := range map[string]Bind{ |
| - "another record's hash": {RecordSHA256: "00", InstanceID: "i-0abc", TofuInstanceID: "i-0abc", Operator: "paul"}, |
| - "tofu names another box": {RecordSHA256: good.RecordSHA256, InstanceID: "i-0abc", TofuInstanceID: "i-0zzz", Operator: "paul"}, |
| - "bind names another box": {RecordSHA256: good.RecordSHA256, InstanceID: "i-0zzz", TofuInstanceID: "i-0zzz", Operator: "paul"}, |
| - "no operator": {RecordSHA256: good.RecordSHA256, InstanceID: "i-0abc", TofuInstanceID: "i-0abc"}, |
| + "another record's hash": {RecordSHA256: "00", InstanceID: "i-0abc", TofuInstanceID: "i-0abc", Operator: "paul"}, |
| + "tofu names another box": {RecordSHA256: good.RecordSHA256, InstanceID: "i-0abc", TofuInstanceID: "i-0zzz", Operator: "paul"}, |
| + "bind names another box": {RecordSHA256: good.RecordSHA256, InstanceID: "i-0zzz", TofuInstanceID: "i-0zzz", Operator: "paul"}, |
| + "no operator": {RecordSHA256: good.RecordSHA256, InstanceID: "i-0abc", TofuInstanceID: "i-0abc"}, |
| } { |
| b := b |
| t.Run(name, func(t *testing.T) { |
| @@ -301,6 +301,19 @@ func TestLoadReadsABindWhenThereIsOne(t *testing.T) { |
| t.Fatalf("bind = %+v, err %v", bind, err) |
| } |
| } |
| + |
| +// Review Focus 5, the storage half: box A's own record, unchanged and |
| +// validly signed, stored under box B's key must not load as B. |
| +func TestLoadRefusesAnotherBoxesRecordUnderThisKey(t *testing.T) { |
| + ctx, st, s := context.Background(), release.NewDirStore(t.TempDir()), newSigner(t) |
| + _, raw := s.record(t, "i-0aaa", "111122223333", "eu-west-1", "i-0aaa") |
| + if _, err := st.Put(ctx, Key("i-0bbb"), raw, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, _, err := Load(ctx, st, "i-0bbb"); !errors.Is(err, ErrUnverified) { |
| + t.Fatalf("err = %v, want ErrUnverified", err) |
| + } |
| +} |
| ``` |
| - [ ] **Step 2: Run the tests to see them fail** |
| @@ -375,6 +388,8 @@ func Publish(ctx context.Context, s release.Store, rec Record) error { |
| // Load returns a box's record, its exact bytes (which a Bind's hash is over) |
| // and its bind when one exists. A missing record is os.ErrNotExist-wrapped. |
| +// A record that names a different box than the key it is stored under is |
| +// ErrUnverified: the key is part of the binding. |
| func Load(ctx context.Context, s release.Store, box string) (Record, []byte, *Bind, error) { |
| raw, _, err := s.Get(ctx, Key(box)) |
| if err != nil { |
| @@ -384,6 +399,11 @@ func Load(ctx context.Context, s release.Store, box string) (Record, []byte, *Bi |
| if err := json.Unmarshal(raw, &rec); err != nil { |
| return Record{}, nil, nil, fmt.Errorf("boxid: %s: %w", box, err) |
| } |
| + if rec.Box != box { |
| + // Another box's record, intact and validly signed, copied under |
| + // this name. Verify would accept it as that other box. |
| + return Record{}, nil, nil, fmt.Errorf("%w: the record stored for %q names %q", ErrUnverified, box, rec.Box) |
| + } |
| braw, _, err := s.Get(ctx, BindKey(box)) |
| if errors.Is(err, os.ErrNotExist) { |
| return rec, raw, nil, nil |
| @@ -1131,7 +1151,7 @@ func TestEligibleBoxesAppliesThePredicateOneBoxAtATime(t *testing.T) { |
| for _, tc := range cases { |
| t.Run(tc.name, func(t *testing.T) { |
| snap := views(t, haBox("i-0aaa", tc.mut)) |
| - got := eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "ie", testKeyFP, nil) |
| + got := eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "eu-west-2", "ie", testKeyFP, nil) |
| if (len(got) == 1) != tc.want { |
| t.Fatalf("eligible = %v, want %v", ids(got), tc.want) |
| } |
| @@ -1142,11 +1162,11 @@ func TestEligibleBoxesAppliesThePredicateOneBoxAtATime(t *testing.T) { |
| func TestEligibleBoxesNeedsAVerifiedRecordFreshnessAndNotLost(t *testing.T) { |
| snap := views(t, haBox("i-0aaa", nil), haBox("i-0bbb", nil), haBox("i-0ccc", nil)) |
| snap = markStale(snap, "i-0bbb") |
| - got := eligibleBoxes(snap, verifiedFor("i-0aaa", "i-0bbb"), "uk", "ie", testKeyFP, nil) |
| + got := eligibleBoxes(snap, verifiedFor("i-0aaa", "i-0bbb"), "uk", "eu-west-2", "ie", testKeyFP, nil) |
| if len(got) != 1 || got[0].BoxID != "i-0aaa" { |
| t.Fatalf("got %v, want only i-0aaa (i-0bbb is stale, i-0ccc has no verified record)", ids(got)) |
| } |
| - got = eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "ie", testKeyFP, map[string]bool{"i-0aaa": true}) |
| + got = eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "eu-west-2", "ie", testKeyFP, map[string]bool{"i-0aaa": true}) |
| if len(got) != 0 { |
| t.Fatalf("a lost box is eligible: %v", ids(got)) |
| } |
| @@ -1158,7 +1178,7 @@ func TestEligibleBoxesNeverCombinesTwoHalfBoxes(t *testing.T) { |
| snap := views(t, |
| haBox("i-0aaa", func(m map[string]any) { m["secrets_key_fp"] = "" }), |
| haBox("i-0bbb", func(m map[string]any) { m["accepts_inbound"] = false })) |
| - if got := eligibleBoxes(snap, verifiedFor("i-0aaa", "i-0bbb"), "uk", "ie", testKeyFP, nil); len(got) != 0 { |
| + if got := eligibleBoxes(snap, verifiedFor("i-0aaa", "i-0bbb"), "uk", "eu-west-2", "ie", testKeyFP, nil); len(got) != 0 { |
| t.Fatalf("got %v, want none", ids(got)) |
| } |
| } |
| @@ -1166,7 +1186,7 @@ func TestEligibleBoxesNeverCombinesTwoHalfBoxes(t *testing.T) { |
| func TestEligibleBoxesAnIncompleteSnapshotOffersNothing(t *testing.T) { |
| snap := views(t, haBox("i-0aaa", nil)) |
| snap.Complete = false |
| - if got := eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "ie", testKeyFP, nil); len(got) != 0 { |
| + if got := eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "eu-west-2", "ie", testKeyFP, nil); len(got) != 0 { |
| t.Fatalf("got %v from an incomplete snapshot", ids(got)) |
| } |
| } |
| @@ -1175,7 +1195,7 @@ func TestEligibleBoxesHomePlacementMatchesTheEmptyCode(t *testing.T) { |
| home := haBox("i-0aaa", func(m map[string]any) { m["placement"] = "" }) |
| named := haBox("i-0bbb", func(m map[string]any) { m["placement"] = "ie" }) |
| snap := views(t, home, named) |
| - got := eligibleBoxes(snap, verifiedFor("i-0aaa", "i-0bbb"), "", "ie", testKeyFP, nil) |
| + got := eligibleBoxes(snap, verifiedFor("i-0aaa", "i-0bbb"), "", "eu-west-2", "ie", testKeyFP, nil) |
| if len(got) != 2 { |
| t.Fatalf("got %v, want both home boxes", ids(got)) |
| } |
| @@ -1202,14 +1222,35 @@ func TestChooseBox(t *testing.T) { |
| t.Fatalf("spread beats load: got %s", got.BoxID) |
| } |
| got, ok := chooseBox([]boxCandidate{a, b}, map[string]boxLoad{"i-0aaa": load(5, "bab/jam"), "i-0bbb": load(2, "bab/jam", "bab/jam")}, "bab/jam") |
| - if !ok || got.BoxID != "i-0aaa" { |
| - t.Fatalf("every box runs the app: fewest of that app, never a refusal; got %s ok=%v", got.BoxID, ok) |
| + if !ok || got.BoxID != "i-0bbb" { |
| + t.Fatalf("every box runs the app: fewest hosts overall, never a refusal; got %s ok=%v", got.BoxID, ok) |
| } |
| got, _ = chooseBox([]boxCandidate{c, b, a}, nil, "bab/jam") |
| if got.BoxID != "i-0aaa" { |
| t.Fatalf("a tie breaks by box id: got %s", got.BoxID) |
| } |
| } |
| + |
| +// A box whose SIGNED region is not the placement's AWS region is not in |
| +// that placement, even when its own file says it is and both regions have |
| +// certificates. |
| +func TestEligibleBoxesRefusesABoxSignedInAnotherRegion(t *testing.T) { |
| + snap := views(t, haBox("i-0aaa", nil), haBox("i-0bbb", nil)) |
| + verified := verifiedFor("i-0aaa", "i-0bbb") |
| + verified["i-0bbb"] = boxid.Identity{AccountID: "1", Region: "us-east-1", InstanceID: "i-0bbb"} |
| + got := eligibleBoxes(snap, verified, "uk", "eu-west-2", "ie", testKeyFP, nil) |
| + if len(got) != 1 || got[0].BoxID != "i-0aaa" { |
| + t.Fatalf("got %v, want only i-0aaa", ids(got)) |
| + } |
| + // And a file that advertises another AWS region than the one expected. |
| + snap = views(t, haBox("i-0aaa", func(m map[string]any) { m["region"] = "us-east-1" })) |
| + if got := eligibleBoxes(snap, verifiedFor("i-0aaa"), "uk", "eu-west-2", "ie", testKeyFP, nil); len(got) != 0 { |
| + t.Fatalf("got %v, want none", ids(got)) |
| + } |
| + if got := eligibleBoxes(views(t, haBox("i-0aaa", nil)), verifiedFor("i-0aaa"), "uk", "", "ie", testKeyFP, nil); len(got) != 0 { |
| + t.Fatalf("got %v with no expected region", ids(got)) |
| + } |
| +} |
| ``` |
| - [ ] **Step 2: Run them to see them fail** |
| @@ -1240,13 +1281,15 @@ type boxCandidate struct { |
| // eligibleBoxes is regionCandidates' predicate applied to one box at a time, |
| // plus what a multi-box region adds: a verified box record that agrees with |
| -// the file's box_id, the HA capability, not draining, not lost. Two boxes |
| +// the file's box_id and whose signed region is awsRegion (the AWS region |
| +// the caller expects for this placement), the HA capability, not draining, |
| +// not lost. Two boxes |
| // that each satisfy half of it never add up to one. |
| // |
| // An incomplete snapshot offers nothing: an unread file could be the one |
| // box that would change the answer. |
| -func eligibleBoxes(snap edgesSnapshot, verified map[string]boxid.Identity, placement, homeName, expectedFP string, lost map[string]bool) []boxCandidate { |
| - if !snap.complete() || expectedFP == "" { |
| +func eligibleBoxes(snap edgesSnapshot, verified map[string]boxid.Identity, placement, awsRegion, homeName, expectedFP string, lost map[string]bool) []boxCandidate { |
| + if !snap.complete() || expectedFP == "" || awsRegion == "" { |
| return nil |
| } |
| var out []boxCandidate |
| @@ -1264,6 +1307,10 @@ func eligibleBoxes(snap edgesSnapshot, verified map[string]boxid.Identity, place |
| case code != want: |
| case !f.Fresh, f.EIP == "", f.BoxID == "": |
| case !ok || id.InstanceID == "": |
| + case id.Region != awsRegion, f.Region != awsRegion: |
| + // The placement code is the box's own word. The AWS region is |
| + // signed: a box whose identity is in another region is not in |
| + // this placement, whatever its file says. |
| case !isTrue(f.HibernationConfigured), !isTrue(f.ServesSharedPool), !isTrue(f.AcceptsInbound): |
| case f.SecretsKeyFP != expectedFP: |
| case !slices.Contains(f.Capabilities, "region-ha-1"): |
| @@ -1286,8 +1333,8 @@ type boxLoad struct { |
| Apps map[string]int // "acct/app" -> hosts of that app |
| } |
| -// chooseBox is spec 1 §5.2's interim rule until spec 2: prefer a box running |
| -// the fewest hosts of this app (soft spread: it never refuses), then the |
| +// chooseBox is spec 1 §5.2's interim rule until spec 2: prefer a box that |
| +// runs no instance of this app (soft spread: it never refuses), then the |
| // fewest hosts overall, then the lowest box id. |
| func chooseBox(cands []boxCandidate, running map[string]boxLoad, app string) (boxCandidate, bool) { |
| if len(cands) == 0 { |
| @@ -1296,9 +1343,10 @@ func chooseBox(cands []boxCandidate, running map[string]boxLoad, app string) (bo |
| best := cands[0] |
| for _, c := range cands[1:] { |
| cl, bl := running[c.BoxID], running[best.BoxID] |
| + cRuns, bRuns := cl.Apps[app] > 0, bl.Apps[app] > 0 |
| switch { |
| - case cl.Apps[app] != bl.Apps[app]: |
| - if cl.Apps[app] < bl.Apps[app] { |
| + case cRuns != bRuns: |
| + if !cRuns { |
| best = c |
| } |
| case cl.Hosts != bl.Hosts: |
| @@ -1878,6 +1926,96 @@ func TestASkippedFileIsRecordedAndReportedOnRestore(t *testing.T) { |
| t.Fatalf("Skipped = %v", st.Skipped) |
| } |
| } |
| + |
| +// A destination that already holds a symlink pointing elsewhere must not |
| +// let a write or a delete reach through it. |
| +func TestRestoreNeverWritesOrDeletesThroughASymlink(t *testing.T) { |
| + ctx, s := context.Background(), release.NewDirStore(t.TempDir()) |
| + p := open(t, s) |
| + stage(t, p, "sub/x", "ours") |
| + stage(t, p, "gone/y", "temp") |
| + commit(t, p) |
| + if err := p.StageDelete("gone/y"); err != nil { |
| + t.Fatal(err) |
| + } |
| + commit(t, p) |
| + |
| + outside, dir := t.TempDir(), t.TempDir() |
| + if err := os.WriteFile(filepath.Join(outside, "y"), []byte("not ours"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, link := range []string{"sub", "gone"} { |
| + if err := os.Symlink(outside, filepath.Join(dir, link)); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if _, _, err := Restore(ctx, s, pfx, dir); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := os.Stat(filepath.Join(outside, "x")); !os.IsNotExist(err) { |
| + t.Fatal("a file was written through the symlink") |
| + } |
| + if got := read(t, outside, "y"); got != "not ours" { |
| + t.Fatalf("a file outside was changed to %q", got) |
| + } |
| + if fi, err := os.Lstat(filepath.Join(dir, "sub")); err != nil || !fi.IsDir() { |
| + t.Fatalf("sub is not a real directory: %v %v", fi, err) |
| + } |
| + if got := read(t, dir, "sub/x"); got != "ours" { |
| + t.Fatalf("sub/x = %q", got) |
| + } |
| +} |
| + |
| +func TestRestoreHandlesADirectoryBecomingAFileAndBack(t *testing.T) { |
| + ctx, s := context.Background(), release.NewDirStore(t.TempDir()) |
| + p := open(t, s) |
| + stage(t, p, "a/b", "child") |
| + commit(t, p) |
| + prior := t.TempDir() // a copy restored while "a" was a directory |
| + if _, _, err := Restore(ctx, s, pfx, prior); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + // "a" becomes a file. |
| + if err := p.StageDelete("a/b"); err != nil { |
| + t.Fatal(err) |
| + } |
| + stage(t, p, "a", "now a file") |
| + commit(t, p) |
| + for name, dir := range map[string]string{"an empty destination": t.TempDir(), "a previously restored one": prior} { |
| + if _, _, err := Restore(ctx, s, pfx, dir); err != nil { |
| + t.Fatalf("%s: %v", name, err) |
| + } |
| + if got := read(t, dir, "a"); got != "now a file" { |
| + t.Fatalf("%s: a = %q", name, got) |
| + } |
| + } |
| + |
| + // And back: "a" becomes a directory again. |
| + if err := p.StageDelete("a"); err != nil { |
| + t.Fatal(err) |
| + } |
| + stage(t, p, "a/c", "child again") |
| + commit(t, p) |
| + for name, dir := range map[string]string{"an empty destination": t.TempDir(), "a previously restored one": prior} { |
| + if _, _, err := Restore(ctx, s, pfx, dir); err != nil { |
| + t.Fatalf("%s: %v", name, err) |
| + } |
| + if got := read(t, dir, "a/c"); got != "child again" { |
| + t.Fatalf("%s: a/c = %q", name, got) |
| + } |
| + } |
| +} |
| + |
| +func TestRestoreIntoAMissingDirectoryIsAnError(t *testing.T) { |
| + ctx, s := context.Background(), release.NewDirStore(t.TempDir()) |
| + p := open(t, s) |
| + stage(t, p, "x", "x") |
| + commit(t, p) |
| + if _, _, err := Restore(ctx, s, pfx, filepath.Join(t.TempDir(), "absent")); err == nil { |
| + t.Fatal("restored into a directory that does not exist") |
| + } |
| +} |
| ``` |
| - [ ] **Step 2: Run them to see them fail** |
| @@ -2169,6 +2307,13 @@ func (p *Publisher) Paths() []string { |
| sort.Strings(out) |
| return out |
| } |
| + |
| +// pending is the last sequence number handed out, committed or not. |
| +func (p *Publisher) pending() int64 { |
| + p.mu.Lock() |
| + defer p.mu.Unlock() |
| + return p.next |
| +} |
| ``` |
| `gofmt` will expand the three one-line methods; let it. |
| @@ -2180,10 +2325,16 @@ package hostfiles |
| import ( |
| "context" |
| + "crypto/rand" |
| + "encoding/hex" |
| + "errors" |
| "fmt" |
| + "io/fs" |
| "os" |
| + "path" |
| "path/filepath" |
| "sort" |
| + "strings" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -2193,50 +2344,74 @@ type RestoreStats struct { |
| Skipped []string // paths the manifest holds as present but unpublished |
| } |
| -// Restore makes dir match the manifest under prefix: it writes every |
| -// published entry, removes every tombstoned one, and reports the skipped |
| +// 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. |
| // |
| -// Every path is validated before anything is written, so a manifest with |
| -// one bad path changes nothing. |
| +// Every path is validated before anything is touched, so a manifest with |
| +// one bad path changes nothing. Every file operation goes through an |
| +// os.Root opened on dir, so nothing is written or removed outside it even |
| +// if dir already holds a symlink pointing elsewhere. |
| func Restore(ctx context.Context, s release.Store, prefix, dir string) (Manifest, RestoreStats, error) { |
| var st RestoreStats |
| m, _, _, err := load(ctx, s, prefix) |
| if err != nil { |
| return Manifest{}, st, err |
| } |
| - paths := make([]string, 0, len(m.Entries)) |
| - for p := range m.Entries { |
| + var live, dead []string |
| + for p, e := range m.Entries { |
| if _, err := CleanPath(p); err != nil { |
| return Manifest{}, st, err |
| } |
| - paths = append(paths, p) |
| + if e.Deleted { |
| + dead = append(dead, p) |
| + } else { |
| + live = append(live, p) |
| + } |
| + } |
| + if len(live)+len(dead) == 0 { |
| + return m, st, nil |
| + } |
| + root, err := os.OpenRoot(dir) |
| + if err != nil { |
| + return Manifest{}, st, fmt.Errorf("hostfiles: restore into %s: %w", dir, err) |
| + } |
| + defer root.Close() |
| + |
| + // Tombstones first, deepest first, so a directory that became a file |
| + // is emptied before the file is written over it. |
| + sort.Slice(dead, func(i, j int) bool { |
| + di, dj := strings.Count(dead[i], "/"), strings.Count(dead[j], "/") |
| + if di != dj { |
| + return di > dj |
| + } |
| + return dead[i] < dead[j] |
| + }) |
| + for _, p := range dead { |
| + removed, err := removeInside(root, p) |
| + if err != nil { |
| + return Manifest{}, st, fmt.Errorf("hostfiles: remove %s: %w", p, err) |
| + } |
| + if removed { |
| + st.Removed++ |
| + } |
| } |
| - sort.Strings(paths) |
| - for _, p := range paths { |
| + sort.Strings(live) |
| + for _, p := range live { |
| e := m.Entries[p] |
| - dst := filepath.Join(dir, filepath.FromSlash(p)) |
| - switch { |
| - case e.Deleted: |
| - err := os.Remove(dst) |
| - if err == nil { |
| - st.Removed++ |
| - } else if !os.IsNotExist(err) { |
| - return Manifest{}, st, fmt.Errorf("hostfiles: remove %s: %w", p, err) |
| - } |
| - case e.Skipped != "": |
| + if e.Skipped != "" { |
| st.Skipped = append(st.Skipped, p) |
| - default: |
| - body, err := fetch(ctx, s, prefix, e) |
| - if err != nil { |
| - return Manifest{}, st, fmt.Errorf("hostfiles: %s: %w", p, err) |
| - } |
| - if err := writeAtomic(dst, body, os.FileMode(e.Mode)); err != nil { |
| - return Manifest{}, st, fmt.Errorf("hostfiles: write %s: %w", p, err) |
| - } |
| - st.Written++ |
| + continue |
| + } |
| + body, err := fetch(ctx, s, prefix, e) |
| + 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 { |
| + return Manifest{}, st, fmt.Errorf("hostfiles: write %s: %w", p, err) |
| } |
| + st.Written++ |
| } |
| return m, st, nil |
| } |
| @@ -2262,36 +2437,125 @@ func fetch(ctx context.Context, s release.Store, prefix string, e Entry) ([]byte |
| return body, nil |
| } |
| -// writeAtomic writes beside the destination and renames into place, so a |
| -// crash leaves the old file or the new one, never part of one. |
| -func writeAtomic(dst string, body []byte, mode os.FileMode) error { |
| +// realParents reports whether every ancestor of rel inside root is a real |
| +// directory: not a symlink, not a file, not missing. |
| +func realParents(root *os.Root, rel string) (bool, error) { |
| + parts := strings.Split(rel, "/") |
| + cur := "" |
| + for _, part := range parts[:len(parts)-1] { |
| + cur = path.Join(cur, part) |
| + fi, err := root.Lstat(filepath.FromSlash(cur)) |
| + if errors.Is(err, fs.ErrNotExist) { |
| + return false, nil |
| + } |
| + if err != nil { |
| + return false, err |
| + } |
| + if !fi.IsDir() { |
| + return false, nil |
| + } |
| + } |
| + return true, nil |
| +} |
| + |
| +// removeInside removes rel if it is a file of ours. A path whose ancestor |
| +// is a symlink, a file or missing is not ours to remove and is left alone: |
| +// that is how a tombstone for "a/b" behaves once "a" has become a file, |
| +// and why a symlinked ancestor never lets a delete reach outside root. |
| +func removeInside(root *os.Root, rel string) (bool, error) { |
| + ok, err := realParents(root, rel) |
| + if err != nil || !ok { |
| + return false, err |
| + } |
| + name := filepath.FromSlash(rel) |
| + fi, err := root.Lstat(name) |
| + if errors.Is(err, fs.ErrNotExist) { |
| + return false, nil |
| + } |
| + if err != nil { |
| + return false, err |
| + } |
| + if fi.IsDir() { |
| + // The path is a directory now: a later entry lives under it, and |
| + // the tombstone is for a file that no longer exists. |
| + return false, nil |
| + } |
| + if err := root.Remove(name); err != nil && !errors.Is(err, fs.ErrNotExist) { |
| + return false, err |
| + } |
| + return true, nil |
| +} |
| + |
| +// writeInside writes rel atomically inside root. Anything in the way that |
| +// is not a real directory (a file where a directory must be, a symlink) |
| +// is removed first, and a directory where the file must be is removed |
| +// with its contents: the manifest is the truth about this tree. |
| +func writeInside(root *os.Root, rel string, body []byte, mode os.FileMode) error { |
| if mode == 0 { |
| mode = 0o600 |
| } |
| - if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { |
| + parent := path.Dir(rel) |
| + if parent != "." { |
| + cur := "" |
| + for _, part := range strings.Split(parent, "/") { |
| + cur = path.Join(cur, part) |
| + fi, err := root.Lstat(filepath.FromSlash(cur)) |
| + if errors.Is(err, fs.ErrNotExist) { |
| + break |
| + } |
| + if err != nil { |
| + return err |
| + } |
| + if !fi.IsDir() { |
| + if err := root.Remove(filepath.FromSlash(cur)); err != nil { |
| + return err |
| + } |
| + break |
| + } |
| + } |
| + if err := root.MkdirAll(filepath.FromSlash(parent), 0o755); err != nil { |
| + return err |
| + } |
| + } |
| + name := filepath.FromSlash(rel) |
| + if fi, err := root.Lstat(name); err == nil && fi.IsDir() { |
| + if err := root.RemoveAll(name); err != nil { |
| + return err |
| + } |
| + } |
| + var rnd [8]byte |
| + if _, err := rand.Read(rnd[:]); err != nil { |
| return err |
| } |
| - tmp, err := os.CreateTemp(filepath.Dir(dst), ".carlos-restore-*") |
| + tmp := filepath.FromSlash(path.Join(parent, ".carlos-restore-"+hex.EncodeToString(rnd[:]))) |
| + f, err := root.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) |
| if err != nil { |
| return err |
| } |
| - defer os.Remove(tmp.Name()) |
| - if _, err := tmp.Write(body); err != nil { |
| - tmp.Close() |
| + done := false |
| + defer func() { |
| + if !done { |
| + f.Close() |
| + root.Remove(tmp) |
| + } |
| + }() |
| + if _, err := f.Write(body); err != nil { |
| return err |
| } |
| - if err := tmp.Chmod(mode); err != nil { |
| - tmp.Close() |
| + if err := f.Chmod(mode); err != nil { |
| return err |
| } |
| - if err := tmp.Sync(); err != nil { |
| - tmp.Close() |
| + if err := f.Sync(); err != nil { |
| return err |
| } |
| - if err := tmp.Close(); err != nil { |
| + if err := f.Close(); err != nil { |
| return err |
| } |
| - return os.Rename(tmp.Name(), dst) |
| + if err := root.Rename(tmp, name); err != nil { |
| + return err |
| + } |
| + done = true |
| + return nil |
| } |
| ``` |
| @@ -2300,7 +2564,9 @@ func writeAtomic(dst string, body []byte, mode os.FileMode) error { |
| Run: `gofmt -w internal/hostfiles && go test ./internal/hostfiles/ -count=1` |
| Expected: PASS. |
| -`TestRestoreRefusesContentThatDoesNotMatchItsHash` overwrites an object through the test store with its etag. That is a test-only tamper: production objects are create-only and Task 11's bucket policy enforces it. |
| +`TestRestoreRefusesContentThatDoesNotMatchItsHash` overwrites an object through the test store with its etag. That is a test-only tamper. In production the publisher only ever creates objects; the bucket policy that makes storage refuse anything else is plan 1b's, because the files prefix depends on the claim's location layout, and it must be in place before the publisher is switched on for any host. |
| + |
| +`Restore` does every file operation through an `os.Root` opened on the destination (Go 1.24+), so a symlink already sitting in the destination cannot carry a write or a delete outside it. The destination must exist. |
| - [ ] **Step 7: Commit** |
| @@ -2316,8 +2582,8 @@ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>" |
| ### Task 6: Publishing a whole directory (`hostfiles.SyncDir`) |
| **Files:** |
| -- Create: `internal/hostfiles/sync.go` |
| -- Test: `internal/hostfiles/sync_test.go` |
| +- Create: `internal/hostfiles/sync.go`, `internal/hostfiles/open_unix.go`, `internal/hostfiles/open_other.go` |
| +- Test: `internal/hostfiles/sync_test.go`, `internal/hostfiles/sync_unix_test.go` |
| **Interfaces:** |
| - Consumes: Task 5's `Publisher`. |
| @@ -2332,8 +2598,11 @@ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>" |
| // on disk. It does not commit. skip, when non-nil, excludes a relative |
| // path (and, for a directory, everything under it). |
| func SyncDir(ctx context.Context, p *Publisher, dir string, skip func(rel string, isDir bool) bool) (SyncStats, error) |
| - // readStable reads one file for publishing. |
| - func readStable(path string) (body []byte, mode uint32, state fileState, err error) |
| + // readStable reads one file, named relative to an opened root, for publishing. |
| + func readStable(root *os.Root, rel string) (body []byte, mode uint32, state fileState, err error) |
| + func syncRoot(ctx context.Context, p *Publisher, root *os.Root, skip func(rel string, isDir bool) bool) (SyncStats, error) |
| + func skipped(skip func(string, bool) bool, rel string, isDir bool) bool // rel or any directory above it is excluded |
| + const openNonblock // O_NONBLOCK on unix, 0 elsewhere |
| type fileState int |
| const ( |
| fileOK fileState = iota |
| @@ -2355,6 +2624,7 @@ package hostfiles |
| import ( |
| "context" |
| + "io" |
| "os" |
| "path/filepath" |
| "reflect" |
| @@ -2465,6 +2735,10 @@ func TestSyncDirRecordsAFileOverTheCapWithoutReadingIt(t *testing.T) { |
| t.Fatal(err) |
| } |
| f.Close() |
| + reads := 0 |
| + orig := readAll |
| + readAll = func(r io.Reader) ([]byte, error) { reads++; return orig(r) } |
| + t.Cleanup(func() { readAll = orig }) |
| p := open(t, s) |
| st, err := SyncDir(ctx, p, src, nil) |
| if err != nil { |
| @@ -2473,6 +2747,9 @@ func TestSyncDirRecordsAFileOverTheCapWithoutReadingIt(t *testing.T) { |
| if !reflect.DeepEqual(st.TooLarge, []string{"huge.bin"}) || st.Published != 0 { |
| t.Fatalf("stats = %+v", st) |
| } |
| + if reads != 0 { |
| + t.Fatalf("the oversized file was read %d times; it must be refused on its size alone", reads) |
| + } |
| commit(t, p) |
| _, rs, err := Restore(ctx, s, pfx, t.TempDir()) |
| if err != nil || !reflect.DeepEqual(rs.Skipped, []string{"huge.bin"}) { |
| @@ -2510,11 +2787,165 @@ func TestSyncDirOfAMissingDirectoryTombstonesNothing(t *testing.T) { |
| t.Fatal("a missing root staged tombstones") |
| } |
| } |
| + |
| +// An excluded directory's files, published before the exclusion applied, |
| +// are left as the manifest has them: not republished, not tombstoned. |
| +func TestSyncDirDoesNotTombstoneWhatASkippedDirectoryHolds(t *testing.T) { |
| + ctx, s, src := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir() |
| + write(t, src, "cache/x", "x") |
| + write(t, src, "keep.txt", "k") |
| + p := open(t, s) |
| + if _, err := SyncDir(ctx, p, src, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + commit(t, p) |
| + skip := func(rel string, isDir bool) bool { return isDir && rel == "cache" } |
| + st, err := SyncDir(ctx, p, src, skip) |
| + if err != nil || st.Deleted != 0 || p.Dirty() { |
| + t.Fatalf("stats = %+v dirty=%v err=%v", st, p.Dirty(), err) |
| + } |
| + if got := p.Paths(); !reflect.DeepEqual(got, []string{"cache/x", "keep.txt"}) { |
| + t.Fatalf("Paths = %v", got) |
| + } |
| +} |
| +``` |
| + |
| +Then `internal/hostfiles/sync_unix_test.go`, for the cases that need FIFOs and symlinks. Two of them use the seam `betweenLookAndOpen` to swap a file after `readStable` has inspected it and before it opens it, which makes the race deterministic: |
| + |
| +```go |
| +//go:build unix |
| + |
| +package hostfiles |
| + |
| +import ( |
| + "context" |
| + "os" |
| + "path/filepath" |
| + "reflect" |
| + "syscall" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// Review Focus 2: special files are skipped, never opened. |
| +func TestSyncDirSkipsAFIFO(t *testing.T) { |
| + ctx, s, src := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir() |
| + write(t, src, "real.txt", "r") |
| + if err := syscall.Mkfifo(filepath.Join(src, "pipe"), 0o600); err != nil { |
| + t.Skipf("no FIFOs here: %v", err) |
| + } |
| + p := open(t, s) |
| + done := make(chan SyncStats, 1) |
| + go func() { |
| + st, _ := SyncDir(ctx, p, src, nil) |
| + done <- st |
| + }() |
| + select { |
| + case st := <-done: |
| + if st.Published != 1 || !reflect.DeepEqual(st.NotRegular, []string{"pipe"}) { |
| + t.Fatalf("stats = %+v", st) |
| + } |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("SyncDir blocked on a FIFO") |
| + } |
| +} |
| + |
| +// A regular file swapped for a FIFO after it was inspected must neither |
| +// block the open nor be read. |
| +func TestReadStableDoesNotBlockOnAFIFOSwappedIn(t *testing.T) { |
| + dir := t.TempDir() |
| + write(t, dir, "f", "regular") |
| + root, err := os.OpenRoot(dir) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer root.Close() |
| + orig := betweenLookAndOpen |
| + t.Cleanup(func() { betweenLookAndOpen = orig }) |
| + betweenLookAndOpen = func() { |
| + full := filepath.Join(dir, "f") |
| + if err := os.Remove(full); err != nil { |
| + t.Error(err) |
| + } |
| + if err := syscall.Mkfifo(full, 0o600); err != nil { |
| + t.Error(err) |
| + } |
| + } |
| + type result struct { |
| + body []byte |
| + state fileState |
| + } |
| + done := make(chan result, 1) |
| + go func() { |
| + body, _, state, _ := readStable(root, "f") |
| + done <- result{body, state} |
| + }() |
| + select { |
| + case r := <-done: |
| + if r.state != fileNotRegular || r.body != nil { |
| + t.Fatalf("state = %v, %d bytes; want fileNotRegular and nothing read", r.state, len(r.body)) |
| + } |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("readStable blocked opening a FIFO") |
| + } |
| +} |
| + |
| +// A parent directory swapped for a symlink to somewhere else, after the |
| +// file was inspected, must not let the read leave the root. |
| +func TestReadStableDoesNotFollowAnAncestorSwappedForASymlink(t *testing.T) { |
| + dir, outside := t.TempDir(), t.TempDir() |
| + write(t, dir, "sub/f", "inside") |
| + if err := os.WriteFile(filepath.Join(outside, "f"), []byte("outside secret"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + root, err := os.OpenRoot(dir) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer root.Close() |
| + orig := betweenLookAndOpen |
| + t.Cleanup(func() { betweenLookAndOpen = orig }) |
| + betweenLookAndOpen = func() { |
| + if err := os.RemoveAll(filepath.Join(dir, "sub")); err != nil { |
| + t.Error(err) |
| + } |
| + if err := os.Symlink(outside, filepath.Join(dir, "sub")); err != nil { |
| + t.Error(err) |
| + } |
| + } |
| + body, _, state, err := readStable(root, "sub/f") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if state != fileNotRegular || body != nil { |
| + t.Fatalf("state = %v, body = %q; the outside file must not be read", state, body) |
| + } |
| +} |
| + |
| +// A symlinked directory present from the start is never descended into. |
| +func TestSyncDirDoesNotDescendASymlinkedDirectory(t *testing.T) { |
| + ctx, s, src, outside := context.Background(), release.NewDirStore(t.TempDir()), t.TempDir(), t.TempDir() |
| + if err := os.WriteFile(filepath.Join(outside, "secret"), []byte("s"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.Symlink(outside, filepath.Join(src, "linkdir")); err != nil { |
| + t.Fatal(err) |
| + } |
| + p := open(t, s) |
| + if _, err := SyncDir(ctx, p, src, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got := p.Paths(); len(got) != 0 { |
| + t.Fatalf("Paths = %v", got) |
| + } |
| +} |
| ``` |
| - [ ] **Step 2: Run them to see them fail** |
| -Run: `go test ./internal/hostfiles/ -run SyncDir -count=1` |
| +Run: `go test ./internal/hostfiles/ -run 'SyncDir|ReadStable' -count=1` |
| Expected: FAIL to build, `undefined: SyncDir`. |
| - [ ] **Step 3: Write `sync.go`** |
| @@ -2524,10 +2955,12 @@ package hostfiles |
| import ( |
| "context" |
| + "errors" |
| "fmt" |
| "io" |
| "io/fs" |
| "os" |
| + "path" |
| "path/filepath" |
| "sort" |
| ) |
| @@ -2542,18 +2975,33 @@ const ( |
| fileTooLarge |
| ) |
| -// readStable reads one file for publishing. It never follows a symlink and |
| -// never opens anything but a regular file. If the file's size, mtime or |
| -// identity changed between the first look and the last byte, the bytes |
| -// read are returned as fileTorn: a possibly inconsistent image the caller |
| -// publishes flagged and reads again later. |
| -func readStable(path string) ([]byte, uint32, fileState, error) { |
| - before, err := os.Lstat(path) |
| - if os.IsNotExist(err) { |
| +// Test seams. betweenLookAndOpen runs after a file has been inspected and |
| +// before it is opened; readAll is the one place file contents are read. |
| +var ( |
| + betweenLookAndOpen = func() {} |
| + readAll = io.ReadAll |
| +) |
| + |
| +// readStable reads one file, named relative to root, for publishing. |
| +// |
| +// It opens through an os.Root, so a path cannot leave the directory even |
| +// if one of its ancestors is swapped for a symlink. It opens without |
| +// blocking, so a FIFO swapped in for the file cannot hang the publisher. |
| +// It reads only if what it opened is the regular file it inspected, and a |
| +// file over MaxFileBytes is refused on its size alone, before any read. |
| +// |
| +// If the file's size, mtime or identity changed between the first look |
| +// and the last byte, the bytes read come back as fileTorn: a possibly |
| +// inconsistent image the caller publishes flagged and reads again later. |
| +func readStable(root *os.Root, rel string) ([]byte, uint32, fileState, error) { |
| + name := filepath.FromSlash(rel) |
| + before, err := root.Lstat(name) |
| + if errors.Is(err, fs.ErrNotExist) { |
| return nil, 0, fileGone, nil |
| } |
| if err != nil { |
| - return nil, 0, fileOK, err |
| + // Includes a path that escapes root through a symlinked ancestor. |
| + return nil, 0, fileNotRegular, nil |
| } |
| if !before.Mode().IsRegular() { |
| return nil, 0, fileNotRegular, nil |
| @@ -2561,32 +3009,36 @@ func readStable(path string) ([]byte, uint32, fileState, error) { |
| if before.Size() > MaxFileBytes { |
| return nil, 0, fileTooLarge, nil |
| } |
| - f, err := os.Open(path) |
| - if os.IsNotExist(err) { |
| + betweenLookAndOpen() |
| + f, err := root.OpenFile(name, os.O_RDONLY|openNonblock, 0) |
| + if errors.Is(err, fs.ErrNotExist) { |
| return nil, 0, fileGone, nil |
| } |
| if err != nil { |
| - return nil, 0, fileOK, err |
| + return nil, 0, fileNotRegular, nil |
| } |
| defer f.Close() |
| opened, err := f.Stat() |
| if err != nil { |
| return nil, 0, fileOK, err |
| } |
| - if !os.SameFile(before, opened) || !opened.Mode().IsRegular() { |
| - // Replaced between the look and the open, possibly by a symlink |
| - // that Open followed. Do not read it. |
| + if !opened.Mode().IsRegular() || !os.SameFile(before, opened) { |
| + // Replaced between the look and the open. Whatever it is now, it |
| + // is not the file that was inspected: do not read it. |
| return nil, 0, fileNotRegular, nil |
| } |
| - body, err := io.ReadAll(io.LimitReader(f, MaxFileBytes+1)) |
| + if opened.Size() > MaxFileBytes { |
| + return nil, 0, fileTooLarge, nil |
| + } |
| + body, err := readAll(io.LimitReader(f, MaxFileBytes+1)) |
| if err != nil { |
| return nil, 0, fileOK, err |
| } |
| if int64(len(body)) > MaxFileBytes { |
| - return nil, 0, fileTooLarge, nil |
| + return nil, 0, fileTooLarge, nil // it grew past the cap while being read |
| } |
| - after, err := os.Lstat(path) |
| state := fileOK |
| + after, err := root.Lstat(name) |
| if err != nil || !os.SameFile(before, after) || after.Size() != before.Size() || !after.ModTime().Equal(before.ModTime()) { |
| state = fileTorn |
| } |
| @@ -2598,35 +3050,55 @@ type SyncStats struct { |
| TooLarge, NotRegular []string |
| } |
| -// SyncDir stages every difference between dir and the manifest. It does |
| -// not commit. A root that cannot be read is an error and stages nothing: |
| -// an unreadable directory is not an empty one. |
| +// skipped reports whether rel, or any directory above it, is excluded. |
| +func skipped(skip func(string, bool) bool, rel string, isDir bool) bool { |
| + if skip == nil { |
| + return false |
| + } |
| + if skip(rel, isDir) { |
| + return true |
| + } |
| + for dir := path.Dir(rel); dir != "."; dir = path.Dir(dir) { |
| + if skip(dir, true) { |
| + return true |
| + } |
| + } |
| + return false |
| +} |
| + |
| +// SyncDir stages every difference between dir and the manifest: changed |
| +// and new regular files, and a tombstone for each manifest path no longer |
| +// on disk. It does not commit. skip, when non-nil, excludes a relative |
| +// path and, for a directory, everything under it; an excluded path is |
| +// left exactly as the manifest has it. |
| +// |
| +// A root that cannot be read is an error and stages nothing: an |
| +// unreadable directory is not an empty one. |
| func SyncDir(ctx context.Context, p *Publisher, dir string, skip func(rel string, isDir bool) bool) (SyncStats, error) { |
| - var st SyncStats |
| - if fi, err := os.Lstat(dir); err != nil { |
| - return st, fmt.Errorf("hostfiles: sync %s: %w", dir, err) |
| - } else if !fi.IsDir() { |
| - return st, fmt.Errorf("hostfiles: sync %s: not a directory", dir) |
| + root, err := os.OpenRoot(dir) |
| + if err != nil { |
| + return SyncStats{}, fmt.Errorf("hostfiles: sync %s: %w", dir, err) |
| } |
| + defer root.Close() |
| + return syncRoot(ctx, p, root, skip) |
| +} |
| + |
| +func syncRoot(ctx context.Context, p *Publisher, root *os.Root, skip func(rel string, isDir bool) bool) (SyncStats, error) { |
| + var st SyncStats |
| seen := map[string]bool{} |
| - err := filepath.WalkDir(dir, func(full string, d fs.DirEntry, err error) error { |
| + err := fs.WalkDir(root.FS(), ".", func(rel string, d fs.DirEntry, err error) error { |
| if err != nil { |
| return err |
| } |
| if err := ctx.Err(); err != nil { |
| return err |
| } |
| - if full == dir { |
| + if rel == "." { |
| return nil |
| } |
| - rel, err := filepath.Rel(dir, full) |
| - if err != nil { |
| - return err |
| - } |
| - rel = filepath.ToSlash(rel) |
| - if skip != nil && skip(rel, d.IsDir()) { |
| + if skipped(skip, rel, d.IsDir()) { |
| if d.IsDir() { |
| - return filepath.SkipDir |
| + return fs.SkipDir |
| } |
| return nil |
| } |
| @@ -2634,8 +3106,8 @@ func SyncDir(ctx context.Context, p *Publisher, dir string, skip func(rel string |
| return nil |
| } |
| // WalkDir does not follow symlinks: a symlink to a directory |
| - // arrives here as a non-directory entry. |
| - body, mode, state, err := readStable(full) |
| + // arrives here as a non-directory entry, and readStable refuses it. |
| + body, mode, state, err := readStable(root, rel) |
| if err != nil { |
| return err |
| } |
| @@ -2661,10 +3133,10 @@ func SyncDir(ctx context.Context, p *Publisher, dir string, skip func(rel string |
| return nil |
| }) |
| if err != nil { |
| - return st, fmt.Errorf("hostfiles: sync %s: %w", dir, err) |
| + return st, fmt.Errorf("hostfiles: sync %s: %w", root.Name(), err) |
| } |
| for _, rel := range p.Paths() { |
| - if seen[rel] || (skip != nil && skip(rel, false)) { |
| + if seen[rel] || skipped(skip, rel, false) { |
| continue |
| } |
| if err := p.StageDelete(rel); err != nil { |
| @@ -2678,17 +3150,39 @@ func SyncDir(ctx context.Context, p *Publisher, dir string, skip func(rel string |
| } |
| ``` |
| -Add to `publisher.go`: |
| +`Publisher.pending()` (in Task 5's `publisher.go`) is the last sequence number handed out; `SyncDir` uses it to count what a `Stage` call really published. |
| + |
| +`internal/hostfiles/open_unix.go`: |
| ```go |
| -// pending is the last sequence number handed out, committed or not. |
| -func (p *Publisher) pending() int64 { |
| - p.mu.Lock() |
| - defer p.mu.Unlock() |
| - return p.next |
| -} |
| +//go:build unix |
| + |
| +package hostfiles |
| + |
| +import "syscall" |
| + |
| +// openNonblock keeps an open from blocking on a FIFO that has no writer. |
| +// It has no effect on a regular file. |
| +const openNonblock = syscall.O_NONBLOCK |
| ``` |
| +`internal/hostfiles/open_other.go`: |
| + |
| +```go |
| +//go:build !unix |
| + |
| +package hostfiles |
| + |
| +const openNonblock = 0 |
| +``` |
| + |
| +Why it is built this way: |
| + |
| +- **`os.Root`** (Go 1.24+) resolves every path beneath one opened directory and refuses any that would leave it, including through a symlink swapped in mid-walk. A path that fails that way is reported as not regular, never read. |
| +- **`O_NONBLOCK`** stops the open from hanging on a FIFO swapped in for the file. The check after the open (`IsRegular` and `SameFile` against the first look) then refuses it unread. |
| +- **Size is checked before any read**, from the first look and again from the opened file. `TestSyncDirRecordsAFileOverTheCapWithoutReadingIt` counts calls to `readAll` and expects none. |
| +- **`skipped`** applies an exclusion to a path and to every directory above it, in the walk and in the tombstone pass alike, so excluding a directory never tombstones what the manifest already holds under it. |
| + |
| `SyncDir` returns an error from the walk before it reaches the tombstone loop, so a directory that vanishes or fails partway never tombstones the files it did not reach. |
| - [ ] **Step 4: Run the tests** |
| @@ -2730,15 +3224,21 @@ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>" |
| // internal: |
| type event struct{ rel string; rescan bool } |
| type watcher interface { |
| - Events() <-chan event |
| - Degraded() bool // some directory has no working watch |
| - Repair() error // try to restore full coverage |
| + Events() <-chan event // closes when the watcher can report nothing more |
| + Degraded() bool // some directory may have no working watch |
| + Repair() error // restore full coverage, or say why not |
| Close() error |
| } |
| func newWatcher(root string, skip func(string, bool) bool) (watcher, error) // per-OS |
| ``` |
| -Spec §5.6, continuous mode: a file is published within one second of its **first** unpublished modification, whether or not it has been closed; a file still being written is published again at the next deadline; an unstable read is published anyway, flagged; overflow triggers a full scan; a missing watch means polling every second until coverage returns. |
| +Spec §5.6, continuous mode: a file is published within one second of its **first** unpublished modification, whether or not it has been closed; a file still being written is published again at the next deadline; an unstable read is published anyway, flagged; a missing watch means polling every second until coverage returns. |
| + |
| +Three failure rules, each with its own test: |
| + |
| +- **Overflow makes coverage uncertain.** A dropped event may have been a directory's creation, which leaves that directory unwatched. So an overflow marks the watcher degraded as well as asking for a scan; the loop then polls and calls `Repair` on every pass until `Repair` succeeds, and scans once more after it does. |
| +- **A dead watcher means polling for good.** If the reader stops, its channel closes. The loop sets the channel and the watcher to nil and polls from then on. It must not spin on the closed channel, and `Repair` on a dead watcher must fail. |
| +- **The first full scan is part of start-up.** Tests wait for it (`startRun` returns after the first `OnStats`) before they change anything, and the event tests run with scans and polling an hour apart, so only the mechanism under test can satisfy them. |
| - [ ] **Step 1: Write the deadline queue's failing test** |
| @@ -2853,10 +3353,11 @@ import ( |
| ) |
| type fakeWatcher struct { |
| - ch chan event |
| - mu sync.Mutex |
| - degraded bool |
| - repairs int |
| + ch chan event |
| + mu sync.Mutex |
| + degraded bool |
| + repairs int |
| + repairErr error // when nil, Repair succeeds and clears degraded |
| } |
| func newFake() *fakeWatcher { return &fakeWatcher{ch: make(chan event, 64)} } |
| @@ -2868,18 +3369,54 @@ func (f *fakeWatcher) Repair() error { |
| f.mu.Lock() |
| defer f.mu.Unlock() |
| f.repairs++ |
| - return nil |
| + if f.repairErr == nil { |
| + f.degraded = false |
| + } |
| + return f.repairErr |
| } |
| func (f *fakeWatcher) setDegraded(v bool) { f.mu.Lock(); f.degraded = v; f.mu.Unlock() } |
| -var fast = Options{Deadline: 40 * time.Millisecond, Scan: time.Hour, Poll: 40 * time.Millisecond} |
| +// eventsOnly publishes on events and nothing else: no repair scan and no |
| +// polling can satisfy a test that uses it. polled is the opposite. |
| +var ( |
| + eventsOnly = Options{Deadline: 40 * time.Millisecond, Scan: time.Hour, Poll: time.Hour} |
| + polled = Options{Deadline: 40 * time.Millisecond, Scan: time.Hour, Poll: 40 * time.Millisecond} |
| +) |
| -// startRun runs the loop against a fake watcher and returns a stop func. |
| +// startRun runs the loop and returns once its first full scan has |
| +// finished, so nothing a test does afterwards can be found by that scan. |
| func startRun(t *testing.T, p *Publisher, dir string, w watcher, o Options) (stop func() error) { |
| t.Helper() |
| + first := make(chan struct{}) |
| + var once sync.Once |
| + o.OnStats = func(SyncStats) { once.Do(func() { close(first) }) } |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + done := make(chan error, 1) |
| + go func() { done <- run(ctx, p, dir, o, w) }() |
| + select { |
| + case <-first: |
| + case err := <-done: |
| + t.Fatalf("run ended before its first scan: %v", err) |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("run never finished its first scan") |
| + } |
| + return func() error { cancel(); return <-done } |
| +} |
| + |
| +// startRunCounting is startRun with a callback on every full scan. |
| +func startRunCounting(t *testing.T, p *Publisher, dir string, w watcher, o Options, onScan func()) (stop func() error) { |
| + t.Helper() |
| + first := make(chan struct{}) |
| + var once sync.Once |
| + o.OnStats = func(SyncStats) { onScan(); once.Do(func() { close(first) }) } |
| ctx, cancel := context.WithCancel(context.Background()) |
| done := make(chan error, 1) |
| go func() { done <- run(ctx, p, dir, o, w) }() |
| + select { |
| + case <-first: |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("run never finished its first scan") |
| + } |
| return func() error { cancel(); return <-done } |
| } |
| @@ -2908,20 +3445,27 @@ func manifestHas(s release.Store, path, sha string) bool { |
| func TestRunPublishesATouchedFileAfterTheDeadline(t *testing.T) { |
| s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| p := open(t, s) |
| - stop := startRun(t, p, dir, w, fast) |
| + stop := startRun(t, p, dir, w, eventsOnly) |
| defer stop() |
| write(t, dir, "instance.key", "k1") |
| + time.Sleep(3 * eventsOnly.Deadline) |
| + if manifestHas(s, "instance.key", sum([]byte("k1"))) { |
| + t.Fatal("published with no event: something other than the event path found it") |
| + } |
| + touched := time.Now() |
| w.ch <- event{rel: "instance.key"} |
| eventually(t, "instance.key published", func() bool { return manifestHas(s, "instance.key", sum([]byte("k1"))) }) |
| + if took := time.Since(touched); took < eventsOnly.Deadline/2 { |
| + t.Fatalf("published %s after the event; the deadline is %s", took, eventsOnly.Deadline) |
| + } |
| } |
| func TestRunPublishesADeleteAsATombstone(t *testing.T) { |
| s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| p := open(t, s) |
| write(t, dir, "a", "a") |
| - stop := startRun(t, p, dir, w, fast) |
| + stop := startRun(t, p, dir, w, eventsOnly) // the first scan publishes "a" |
| defer stop() |
| - w.ch <- event{rel: "a"} |
| eventually(t, "a published", func() bool { return manifestHas(s, "a", sum([]byte("a"))) }) |
| if err := os.Remove(filepath.Join(dir, "a")); err != nil { |
| t.Fatal(err) |
| @@ -2936,30 +3480,71 @@ func TestRunPublishesADeleteAsATombstone(t *testing.T) { |
| func TestRunRescansOnOverflow(t *testing.T) { |
| s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| p := open(t, s) |
| - stop := startRun(t, p, dir, w, fast) |
| + stop := startRun(t, p, dir, w, eventsOnly) |
| defer stop() |
| write(t, dir, "missed.txt", "m") // no event for it: the queue overflowed |
| + time.Sleep(3 * eventsOnly.Deadline) |
| + if manifestHas(s, "missed.txt", sum([]byte("m"))) { |
| + t.Fatal("found before the rescan was asked for") |
| + } |
| w.ch <- event{rescan: true} |
| eventually(t, "missed.txt found by the rescan", func() bool { return manifestHas(s, "missed.txt", sum([]byte("m"))) }) |
| } |
| // A2-5 residual: with a watch missing, changes are found by polling, not |
| // by waiting for the ten-minute scan. |
| -func TestRunPollsWhileTheWatcherIsDegradedAndTriesToRepair(t *testing.T) { |
| +func TestRunPollsWhileTheWatcherIsDegradedAndKeepsTryingToRepair(t *testing.T) { |
| s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| w.setDegraded(true) |
| + w.repairErr = errors.New("watch limit reached") |
| p := open(t, s) |
| - stop := startRun(t, p, dir, w, fast) |
| + stop := startRun(t, p, dir, w, polled) |
| defer stop() |
| write(t, dir, "quiet.txt", "q") // no event at all |
| eventually(t, "quiet.txt found by polling", func() bool { return manifestHas(s, "quiet.txt", sum([]byte("q"))) }) |
| - eventually(t, "a repair attempt", func() bool { w.mu.Lock(); defer w.mu.Unlock(); return w.repairs > 0 }) |
| + eventually(t, "more than one repair attempt", func() bool { w.mu.Lock(); defer w.mu.Unlock(); return w.repairs > 1 }) |
| +} |
| + |
| +// A2-5 residual, the other half: once repair succeeds, polling stops and |
| +// what changed meanwhile is found by one scan. |
| +func TestRunScansOnceAfterARepairThenStopsPolling(t *testing.T) { |
| + s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| + p := open(t, s) |
| + scans := 0 |
| + var mu sync.Mutex |
| + o := eventsOnly |
| + stop := startRunCounting(t, p, dir, w, o, func() { mu.Lock(); scans++; mu.Unlock() }) |
| + defer stop() |
| + write(t, dir, "during.txt", "d") |
| + w.setDegraded(true) // the fake's Repair succeeds and clears it |
| + eventually(t, "during.txt found by the post-repair scan", func() bool { return manifestHas(s, "during.txt", sum([]byte("d"))) }) |
| + mu.Lock() |
| + n := scans |
| + mu.Unlock() |
| + time.Sleep(5 * o.Deadline) |
| + mu.Lock() |
| + defer mu.Unlock() |
| + if scans != n { |
| + t.Fatalf("%d more scans after the repair; polling should have stopped", scans-n) |
| + } |
| +} |
| + |
| +// A watcher whose reader dies closes its channel. The loop must fall back |
| +// to polling, not spin and not stop publishing. |
| +func TestRunPollsAfterTheWatcherStops(t *testing.T) { |
| + s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| + p := open(t, s) |
| + stop := startRun(t, p, dir, w, polled) |
| + defer stop() |
| + close(w.ch) |
| + write(t, dir, "after.txt", "a") |
| + eventually(t, "after.txt found by polling", func() bool { return manifestHas(s, "after.txt", sum([]byte("a"))) }) |
| } |
| func TestRunWithNoWatcherPolls(t *testing.T) { |
| s, dir := release.NewDirStore(t.TempDir()), t.TempDir() |
| p := open(t, s) |
| - stop := startRun(t, p, dir, nil, fast) |
| + stop := startRun(t, p, dir, nil, polled) |
| defer stop() |
| write(t, dir, "x", "x") |
| eventually(t, "x found by polling", func() bool { return manifestHas(s, "x", sum([]byte("x"))) }) |
| @@ -2971,7 +3556,7 @@ func TestRunStopsWhenSuperseded(t *testing.T) { |
| ctx, cancel := context.WithCancel(context.Background()) |
| defer cancel() |
| done := make(chan error, 1) |
| - go func() { done <- run(ctx, p, dir, fast, w) }() |
| + go func() { done <- run(ctx, p, dir, eventsOnly, w) }() |
| stage(t, other, "theirs", "t") |
| commit(t, other) |
| write(t, dir, "mine", "m") |
| @@ -2989,7 +3574,7 @@ func TestRunStopsWhenSuperseded(t *testing.T) { |
| func TestRunIgnoresAnEventForAPathThatEscapes(t *testing.T) { |
| s, dir, w := release.NewDirStore(t.TempDir()), t.TempDir(), newFake() |
| p := open(t, s) |
| - stop := startRun(t, p, dir, w, fast) |
| + stop := startRun(t, p, dir, w, eventsOnly) |
| w.ch <- event{rel: "../outside"} |
| write(t, dir, "ok", "ok") |
| w.ch <- event{rel: "ok"} |
| @@ -3013,17 +3598,18 @@ package hostfiles |
| import ( |
| "context" |
| "errors" |
| + "fmt" |
| "log" |
| - "path/filepath" |
| + "os" |
| "time" |
| ) |
| type Options struct { |
| - Deadline time.Duration |
| - Scan time.Duration |
| - Poll time.Duration |
| + Deadline time.Duration // first unpublished modification to publish |
| + Scan time.Duration // repair scan while every directory is watched |
| + Poll time.Duration // full-scan period while watch coverage is missing |
| Skip func(rel string, isDir bool) bool |
| - OnStats func(SyncStats) |
| + OnStats func(SyncStats) // called after each full scan; may be nil |
| } |
| // DefaultOptions are region HA spec 1 §5.6's values. |
| @@ -3036,29 +3622,35 @@ type event struct { |
| rescan bool |
| } |
| +// watcher reports changes under one directory. Its event channel closes |
| +// when it can report nothing more. |
| type watcher interface { |
| Events() <-chan event |
| - Degraded() bool |
| - Repair() error |
| + Degraded() bool // some directory may have no working watch |
| + Repair() error // restore full coverage, or say why not |
| Close() error |
| } |
| // Run publishes dir's changes until ctx ends (it returns ctx.Err()) or |
| -// another writer supersedes the manifest (ErrSuperseded). Where the OS has |
| -// no watcher, it polls. |
| +// another writer supersedes the manifest (ErrSuperseded). Where there is |
| +// no watcher, or the watcher fails, it polls. |
| func Run(ctx context.Context, p *Publisher, dir string, o Options) error { |
| w, err := newWatcher(dir, o.Skip) |
| if err != nil { |
| log.Printf("hostfiles: no watcher for %s (%v): polling every %s", dir, err, o.Poll) |
| - w = nil |
| - } |
| - if w != nil { |
| - defer w.Close() |
| + return run(ctx, p, dir, o, nil) |
| } |
| + defer w.Close() |
| return run(ctx, p, dir, o, w) |
| } |
| func run(ctx context.Context, p *Publisher, dir string, o Options, w watcher) error { |
| + root, err := os.OpenRoot(dir) |
| + if err != nil { |
| + return fmt.Errorf("hostfiles: %s: %w", dir, err) |
| + } |
| + defer root.Close() |
| + |
| var events <-chan event |
| if w != nil { |
| events = w.Events() |
| @@ -3073,7 +3665,14 @@ func run(ctx context.Context, p *Publisher, dir string, o Options, w watcher) er |
| select { |
| case <-ctx.Done(): |
| return ctx.Err() |
| - case ev := <-events: |
| + case ev, open := <-events: |
| + if !open { |
| + // The watcher has stopped for good. A nil channel never |
| + // fires, and with no watcher every pass below polls. |
| + log.Printf("hostfiles: watcher for %s stopped: polling every %s", dir, o.Poll) |
| + events, w = nil, nil |
| + continue |
| + } |
| if ev.rescan { |
| needScan = true |
| } else { |
| @@ -3083,6 +3682,13 @@ func run(ctx context.Context, p *Publisher, dir string, o Options, w watcher) er |
| case <-tick.C: |
| } |
| now := time.Now() |
| + if w != nil && w.Degraded() { |
| + if err := w.Repair(); err != nil { |
| + log.Printf("hostfiles: %s: watch coverage still missing: %v", dir, err) |
| + } else { |
| + needScan = true // whatever changed while coverage was missing |
| + } |
| + } |
| polling := w == nil || w.Degraded() |
| switch { |
| case polling && now.Sub(lastScan) >= o.Poll: |
| @@ -3091,12 +3697,7 @@ func run(ctx context.Context, p *Publisher, dir string, o Options, w watcher) er |
| needScan = true |
| } |
| if needScan { |
| - if w != nil && w.Degraded() { |
| - if err := w.Repair(); err != nil { |
| - log.Printf("hostfiles: %s: watch coverage still missing: %v", dir, err) |
| - } |
| - } |
| - st, err := SyncDir(ctx, p, dir, o.Skip) |
| + st, err := syncRoot(ctx, p, root, o.Skip) |
| if err != nil { |
| if ctx.Err() != nil { |
| return ctx.Err() |
| @@ -3110,7 +3711,7 @@ func run(ctx context.Context, p *Publisher, dir string, o Options, w watcher) er |
| } |
| } |
| for _, rel := range q.Due(now) { |
| - again, err := publishOne(ctx, p, dir, rel, o.Skip) |
| + again, err := publishOne(ctx, p, root, rel, o.Skip) |
| if err != nil { |
| log.Printf("hostfiles: %s: %v", rel, err) |
| q.Touch(rel, now) // try again at the next deadline |
| @@ -3133,20 +3734,23 @@ func run(ctx context.Context, p *Publisher, dir string, o Options, w watcher) er |
| // publishOne stages one path's current state. again is true when the file |
| // changed while it was read: its image is published flagged, and the |
| // caller reads it again at the next deadline. |
| -func publishOne(ctx context.Context, p *Publisher, dir, rel string, skip func(string, bool) bool) (again bool, err error) { |
| +func publishOne(ctx context.Context, p *Publisher, root *os.Root, rel string, skip func(string, bool) bool) (again bool, err error) { |
| clean, err := CleanPath(rel) |
| if err != nil { |
| return false, nil // a path the watcher should never have produced: skip it |
| } |
| - if skip != nil && skip(clean, false) { |
| + if skipped(skip, clean, false) { |
| return false, nil |
| } |
| - body, mode, state, err := readStable(filepath.Join(dir, filepath.FromSlash(clean))) |
| + body, mode, state, err := readStable(root, clean) |
| if err != nil { |
| return false, err |
| } |
| switch state { |
| case fileGone, fileNotRegular: |
| + // Gone, or no longer a regular file (a directory, a link): either |
| + // way the manifest's file at this path is no more. StageDelete is |
| + // a no-op for a path the manifest never had. |
| return false, p.StageDelete(clean) |
| case fileTooLarge: |
| return false, p.StageSkipped(clean, "too-large") |
| @@ -3195,20 +3799,36 @@ import ( |
| "testing" |
| "time" |
| + "golang.org/x/sys/unix" |
| + |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| -var realFast = Options{Deadline: 50 * time.Millisecond, Scan: time.Hour, Poll: time.Hour} |
| +// realWatcher returns a working inotify watcher, or skips the test where |
| +// the kernel will not give one (a shared machine at its per-user limit). |
| +// CI must not skip: see the plan's Task 7, step 10. |
| +func realWatcher(t *testing.T, root string, skip func(string, bool) bool) watcher { |
| + t.Helper() |
| + w, err := newWatcher(root, skip) |
| + if err != nil { |
| + if os.Getenv("CARLOS_REQUIRE_INOTIFY") != "" { |
| + t.Fatalf("inotify is required here: %v", err) |
| + } |
| + t.Skipf("no inotify instance available: %v", err) |
| + } |
| + t.Cleanup(func() { w.Close() }) |
| + return w |
| +} |
| +// startReal runs the loop on a real watcher with scans and polling off, |
| +// and returns once the first full scan is done: everything after that is |
| +// found by an inotify event or not at all. |
| func startReal(t *testing.T, s release.Store, dir string) { |
| t.Helper() |
| + w := realWatcher(t, dir, nil) |
| p := open(t, s) |
| - ctx, cancel := context.WithCancel(context.Background()) |
| - done := make(chan error, 1) |
| - go func() { done <- Run(ctx, p, dir, realFast) }() |
| - t.Cleanup(func() { cancel(); <-done }) |
| - // Let the first full scan finish, so what follows is found by events. |
| - time.Sleep(100 * time.Millisecond) |
| + stop := startRun(t, p, dir, w, Options{Deadline: 50 * time.Millisecond, Scan: time.Hour, Poll: time.Hour}) |
| + t.Cleanup(func() { stop() }) |
| } |
| func TestWatcherPublishesAFileStillHeldOpen(t *testing.T) { |
| @@ -3260,11 +3880,7 @@ func TestWatcherDoesNotWatchASkippedDirectory(t *testing.T) { |
| if err := os.MkdirAll(filepath.Join(root, "cache"), 0o755); err != nil { |
| t.Fatal(err) |
| } |
| - w, err := newWatcher(root, func(rel string, isDir bool) bool { return isDir && rel == "cache" }) |
| - if err != nil { |
| - t.Fatal(err) |
| - } |
| - defer w.Close() |
| + w := realWatcher(t, root, func(rel string, isDir bool) bool { return isDir && rel == "cache" }) |
| write(t, root, "cache/x", "x") |
| select { |
| case ev := <-w.Events(): |
| @@ -3272,6 +3888,71 @@ func TestWatcherDoesNotWatchASkippedDirectory(t *testing.T) { |
| case <-time.After(150 * time.Millisecond): |
| } |
| } |
| + |
| +// A2-8: a kernel overflow may have swallowed a directory's creation, so |
| +// coverage is uncertain until Repair has walked the tree again. |
| +func TestWatcherOverflowMarksCoverageUncertainUntilRepaired(t *testing.T) { |
| + root := t.TempDir() |
| + w := realWatcher(t, root, nil).(*inotifyWatcher) |
| + // The directory appears while events are being dropped: no watch on it. |
| + if err := os.MkdirAll(filepath.Join(root, "lost"), 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + for len(w.ch) > 0 { |
| + <-w.ch |
| + } |
| + w.mu.Lock() |
| + for wd, rel := range w.wds { |
| + if rel == "lost" { |
| + delete(w.wds, wd) // as if its IN_CREATE had been dropped |
| + } |
| + } |
| + w.mu.Unlock() |
| + w.handle(0, unix.IN_Q_OVERFLOW, "") |
| + if !w.Degraded() { |
| + t.Fatal("an overflow left coverage marked complete") |
| + } |
| + if ev := <-w.Events(); !ev.rescan { |
| + t.Fatalf("event = %+v, want a rescan", ev) |
| + } |
| + if err := w.Repair(); err != nil { |
| + t.Fatal(err) |
| + } |
| + if w.Degraded() { |
| + t.Fatal("still degraded after a successful repair") |
| + } |
| + write(t, root, "lost/later.txt", "l") |
| + deadline := time.After(2 * time.Second) |
| + for { |
| + select { |
| + case ev := <-w.Events(): |
| + if ev.rel == "lost/later.txt" { |
| + return |
| + } |
| + case <-deadline: |
| + t.Fatal("no event for a write inside the directory whose creation was lost") |
| + } |
| + } |
| +} |
| + |
| +func TestAStoppedWatcherCannotBeRepairedAndClosesItsChannel(t *testing.T) { |
| + w := realWatcher(t, t.TempDir(), nil).(*inotifyWatcher) |
| + w.Close() |
| + select { |
| + case _, open := <-w.Events(): |
| + if open { |
| + t.Fatal("an event after Close") |
| + } |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("the channel did not close") |
| + } |
| + if err := w.Repair(); err == nil { |
| + t.Fatal("a stopped watcher reported a successful repair") |
| + } |
| + if !w.Degraded() { |
| + t.Fatal("a stopped watcher is not degraded") |
| + } |
| +} |
| ``` |
| `internal/hostfiles/watch_linux.go`: |
| @@ -3306,7 +3987,8 @@ type inotifyWatcher struct { |
| mu sync.Mutex |
| wds map[int32]string // watch descriptor -> directory, relative to root ("" is root) |
| - degraded atomic.Bool |
| + degraded atomic.Bool // some directory may be unwatched |
| + dead atomic.Bool // the reader has stopped; no event will ever arrive |
| } |
| func newWatcher(root string, skip func(string, bool) bool) (watcher, error) { |
| @@ -3329,7 +4011,11 @@ func (w *inotifyWatcher) Close() error { return w.f.Close() } |
| // Repair re-adds a watch on every directory. InotifyAddWatch on a path |
| // already watched returns the same descriptor, so this is safe to repeat. |
| +// A watcher whose reader has stopped cannot be repaired. |
| func (w *inotifyWatcher) Repair() error { |
| + if w.dead.Load() { |
| + return errors.New("the inotify reader has stopped") |
| + } |
| if err := w.addTree(""); err != nil { |
| return err |
| } |
| @@ -3387,7 +4073,9 @@ func (w *inotifyWatcher) send(ev event) { |
| case w.ch <- ev: |
| default: |
| // Our own buffer is full: the same remedy as a kernel overflow. |
| - // A rescan request must not be lost, so make room for it. |
| + // A path was dropped, so only a scan finds it now; and it may |
| + // have been a new directory, so coverage is uncertain too. |
| + w.degraded.Store(true) |
| select { |
| case <-w.ch: |
| default: |
| @@ -3405,9 +4093,10 @@ func (w *inotifyWatcher) loop() { |
| for { |
| n, err := w.f.Read(buf) |
| if err != nil { |
| - if !errors.Is(err, os.ErrClosed) { |
| - w.degraded.Store(true) |
| - } |
| + // Closed by us, or broken. Either way nothing more will be |
| + // read: the closed channel tells Run to poll from here on. |
| + w.dead.Store(true) |
| + w.degraded.Store(true) |
| return |
| } |
| for off := 0; off+unix.SizeofInotifyEvent <= n; { |
| @@ -3432,6 +4121,10 @@ func (w *inotifyWatcher) loop() { |
| func (w *inotifyWatcher) handle(wd int32, mask uint32, name string) { |
| if mask&unix.IN_Q_OVERFLOW != 0 { |
| + // Events were dropped, and one of them may have been a new |
| + // directory, which is then unwatched. Coverage is uncertain until |
| + // Repair has walked the tree again. |
| + w.degraded.Store(true) |
| w.send(event{rescan: true}) |
| return |
| } |
| @@ -3475,7 +4168,13 @@ func (w *inotifyWatcher) handle(wd int32, mask uint32, name string) { |
| Run: `gofmt -l internal/hostfiles; go vet ./internal/hostfiles/ && go test ./internal/hostfiles/ -count=1 -race && GOOS=darwin go build ./internal/hostfiles/ && GOOS=windows go build ./internal/hostfiles/` |
| Expected: no gofmt output, PASS, both cross-builds succeed. |
| -If `TestWatcherPublishesAFileStillHeldOpen` is flaky under `-race` on a loaded machine, do not raise `eventually`'s two-second bound past five seconds: look for a lost event first (a `send` that dropped a path rather than requesting a rescan). |
| +**The inotify tests skip where the kernel refuses an inotify instance**, and that hides them. Check they ran: `go test ./internal/hostfiles/ -run Watcher -count=1 -v | grep -c SKIP` must print `0`. On the shared development machine it will not: `fs.inotify.max_user_instances` is 128 and the many agent sessions there use them all (this is how the plan's own dry run went: every test here passed except the six real-inotify ones, which skipped). So run them once somewhere with a free instance, with skipping turned off, before this task is called done: |
| + |
| + CARLOS_REQUIRE_INOTIFY=1 go test ./internal/hostfiles/ -run Watcher -count=1 -race -v |
| + |
| +Expected: six tests `PASS`, none `SKIP`. The dev box is the place; ask Paul for the session. Until that run is green, say in the task's report that `watch_linux.go` is unverified against a real kernel. Do not raise the machine's inotify limit yourself: that is Paul's call. |
| + |
| +If `TestWatcherPublishesAFileStillHeldOpen` is flaky under `-race`, do not raise `eventually`'s two-second bound past five seconds: look for a lost event first. |
| - [ ] **Step 11: Commit** |
| @@ -3572,12 +4271,15 @@ func TestReplaceTimersIsAllOrNothing(t *testing.T) { |
| if err := r.UpsertTimer(Timer{Host: "a.test", Name: "keep", Kind: "schedule", Spec: "@every 1m", Path: "/t", NextAt: 100, CreatedAt: 1}); err != nil { |
| t.Fatal(err) |
| } |
| - bad := []Timer{ |
| - {Host: "a.test", Name: "x", Kind: "schedule", Spec: "@every 1m", Path: "/t", NextAt: 1, CreatedAt: 1}, |
| - {Host: "elsewhere.test", Name: "y", Kind: "schedule", Spec: "@every 1m", Path: "/t", NextAt: 1, CreatedAt: 1}, |
| + // Two rows with one name: the delete and the first insert succeed, the |
| + // second insert breaks the primary key. Without a transaction the host |
| + // would be left with "dup" and without "keep". |
| + dup := []Timer{ |
| + {Host: "a.test", Name: "dup", Kind: "schedule", Spec: "@every 1m", Path: "/t", NextAt: 1, CreatedAt: 1}, |
| + {Host: "a.test", Name: "dup", Kind: "schedule", Spec: "@every 2m", Path: "/t", NextAt: 1, CreatedAt: 1}, |
| } |
| - if err := r.ReplaceTimers("a.test", bad); err == nil { |
| - t.Fatal("a row for another host was accepted") |
| + if err := r.ReplaceTimers("a.test", dup); err == nil { |
| + t.Fatal("two rows with one name were accepted") |
| } |
| got, _ := r.TimersFor("a.test") |
| if len(got) != 1 || got[0].Name != "keep" { |
| @@ -3585,6 +4287,14 @@ func TestReplaceTimersIsAllOrNothing(t *testing.T) { |
| } |
| } |
| +func TestReplaceTimersRefusesARowForAnotherHost(t *testing.T) { |
| + r := openTimersReg(t) |
| + rows := []Timer{{Host: "elsewhere.test", Name: "y", Kind: "schedule", Spec: "@every 1m", Path: "/t", NextAt: 1, CreatedAt: 1}} |
| + if err := r.ReplaceTimers("a.test", rows); err == nil { |
| + t.Fatal("a row for another host was accepted") |
| + } |
| +} |
| + |
| func TestReplaceTimersWithNoRowsClearsTheHost(t *testing.T) { |
| r := openTimersReg(t) |
| if err := r.UpsertTimer(Timer{Host: "a.test", Name: "gone", Kind: "schedule", Spec: "@every 1m", Path: "/t", NextAt: 100, CreatedAt: 1}); err != nil { |
| @@ -3786,6 +4496,24 @@ func TestExportWakeOfAnUnknownHostIsAnError(t *testing.T) { |
| t.Fatal("exported wake state for a host with no route") |
| } |
| } |
| + |
| +func TestImportWakeOfTheDefaultClassReplacesAnotherClass(t *testing.T) { |
| + r := wakeReg(t) |
| + seedHost(t, r, "shop.test") |
| + if err := r.SetReplication("shop.test", registry.ReplicationDaily); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := ImportWake(r, "shop.test", WakeState{V: 1, Host: "shop.test", Replication: ""}); err != nil { |
| + t.Fatal(err) |
| + } |
| + rt, err := r.Get("shop.test") |
| + if err != nil || rt == nil { |
| + t.Fatalf("route: %v %v", rt, err) |
| + } |
| + if rt.Replication != "" { |
| + t.Fatalf("Replication = %q, want the default", rt.Replication) |
| + } |
| +} |
| ``` |
| - [ ] **Step 5: Run them to see them fail** |
| @@ -3892,10 +4620,10 @@ func ImportWake(reg *registry.Registry, host string, ws WakeState) error { |
| if err := reg.SetNextDue(host, ws.NextDue); err != nil { |
| return err |
| } |
| - if ws.Replication != "" { |
| - return reg.SetReplication(host, ws.Replication) |
| - } |
| - return nil |
| + // Always, the empty class included: "" is the default rung, and a |
| + // destination left on its previous class would not be the state that |
| + // was exported. |
| + return reg.SetReplication(host, ws.Replication) |
| } |
| // StageWake stages the wake state as a platform item. Unchanged state |
| @@ -4063,10 +4791,16 @@ func TestLookalikesAreNotEvidence(t *testing.T) { |
| } |
| func TestAnUnverifiedIdentityIsNeverFenced(t *testing.T) { |
| - o := FromDescribe(describe("", "", types.InstanceStateNameTerminated), "", "") |
| + // A found, terminated observation whose every coordinate is empty, so |
| + // it MATCHES an empty identity field for field. Only the guard on the |
| + // identity itself can refuse it. |
| + o := Observation{Via: ViaDescribe, Found: true, State: "terminated"} |
| if _, err := Verify("i-0abc", boxid.Identity{}, o, now); !errors.Is(err, ErrNotEvidence) { |
| t.Fatalf("err = %v: an empty identity matched an empty observation", err) |
| } |
| + if _, err := Verify("", id, Observation{Via: ViaDescribe, Found: true, InstanceID: "i-0abc", Account: "111122223333", Region: "eu-west-1", State: "terminated"}, now); !errors.Is(err, ErrNotEvidence) { |
| + t.Fatalf("err = %v: a fence with no box name", err) |
| + } |
| } |
| const terminatedEvent = `{"version":"0","id":"ev-123","detail-type":"EC2 Instance State-change Notification","source":"aws.ec2","account":"111122223333","time":"2026-10-05T11:59:00Z","region":"eu-west-1","resources":["arn:aws:ec2:eu-west-1:111122223333:instance/i-0abc"],"detail":{"instance-id":"i-0abc","state":"terminated"}}` |
| @@ -4087,10 +4821,10 @@ func TestATerminatedEventIsAFence(t *testing.T) { |
| func TestEventsThatAreNotEC2StateChangesAreRefused(t *testing.T) { |
| for name, body := range map[string]string{ |
| - "not JSON": `terminated`, |
| - "another source": `{"id":"e","detail-type":"EC2 Instance State-change Notification","source":"my.app","account":"111122223333","region":"eu-west-1","detail":{"instance-id":"i-0abc","state":"terminated"}}`, |
| + "not JSON": `terminated`, |
| + "another source": `{"id":"e","detail-type":"EC2 Instance State-change Notification","source":"my.app","account":"111122223333","region":"eu-west-1","detail":{"instance-id":"i-0abc","state":"terminated"}}`, |
| "another detail-type": `{"id":"e","detail-type":"EC2 Spot Instance Interruption Warning","source":"aws.ec2","account":"111122223333","region":"eu-west-1","detail":{"instance-id":"i-0abc","state":"terminated"}}`, |
| - "no event id": `{"detail-type":"EC2 Instance State-change Notification","source":"aws.ec2","account":"111122223333","region":"eu-west-1","detail":{"instance-id":"i-0abc","state":"terminated"}}`, |
| + "no event id": `{"detail-type":"EC2 Instance State-change Notification","source":"aws.ec2","account":"111122223333","region":"eu-west-1","detail":{"instance-id":"i-0abc","state":"terminated"}}`, |
| } { |
| t.Run(name, func(t *testing.T) { |
| if _, err := FromEvent([]byte(body)); err == nil { |
| @@ -4153,9 +4887,9 @@ const ( |
| ViaDescribe = "describe" |
| ViaEvent = "event" |
| - stateTerminated = "terminated" |
| - eventSource = "aws.ec2" |
| - eventDetailType = "EC2 Instance State-change Notification" |
| + stateTerminated = "terminated" |
| + eventSource = "aws.ec2" |
| + eventDetailType = "EC2 Instance State-change Notification" |
| ) |
| // ErrNotEvidence wraps every reason an observation does not fence a box. |