amadan
hostfiles: a scan retires an inherited skipped entry past the cap, and Restore never walks a tombstone past it (Astra's review of 28d086d8, two P2s)
1. StageDelete takes no cap. It only tombstones an entry the manifest already holds, and a tombstone is exempt. Before this, a skipped entry past the cap, inherited, had no file after a restore, and every scan's StageDelete of it failed with ErrTooDeep, so Publish and the release flush never committed. 2. Restore's partition drops a tombstone past the cap. Removing it would resolve every component of a path no restore could have written: cubic in depth with surviving parents, and not cancellable. Anything still standing there is left for the next walk, which refuses it (ErrTooDeep), as before. New writes still refuse a path past the cap: Stage, StageSkipped, publishOne and the walk. The MaxPathBytes comment now says which writers apply the cap and how. Tests (each fails without its fix): - TestAScanRetiresAnInheritedSkippedEntryPastTheCap: restore, then scan, then commit. - TestRestoreNeverWalksATombstonePastTheCap: a real 2049-level tree at the destination is left untouched. With the walk restored, the test is still walking at go test's 10-minute timeout. - TestRunStopsOnAManifestItCannotCommit skips its deep path, so it still reaches the stop branch now that a scan can retire such an entry. Census: the Plan 14 hostfiles envelope is updated in its overlay. No root-read unit changed, and the legacy seal stands. Astra's review is committed unedited.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7 files changed,
+152
−17
docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP.md+27 −0internal/hostfiles/fix4_linux_test.go+4 −2internal/hostfiles/fix5_linux_test.go+98 −3internal/hostfiles/publisher.go+5 −4internal/hostfiles/restore.go+9 −2internal/hostfiles/sync.go+7 −4internal/provision/testdata/plan14-root-reads-hostfiles-cap-236.tsv+2 −2
CI cancelled — superseded by a newer push — run details
CI log
=== gate === cd launchctx && go vet ./... && go test ./... -count=1 ok amadan.net/carlos/platform.git/launchctx 0.014s ? amadan.net/carlos/platform.git/launchctx/controlname [no test files] ? amadan.net/carlos/platform.git/launchctx/wireid [no test files] go vet ./... gofmt -l .: clean go test ./... -count=1 -timeout 30m amadan: job cancelled — a newer push superseded it
diff --git a/docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP.md b/docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP.md| new file mode 100644 |
| index 000000000..0d1b847d0 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP.md |
| @@ -0,0 +1,27 @@ |
| +--- |
| + |
| +**FIRST PROMPT:** |
| +Review `hostfiles-cap-live-entries` at `7d4efe54`, read-only, following the supplied brief. |
| + |
| +**WHAT:** |
| +Assess correctness, test coverage, census changes, and readiness to merge after the freeze. |
| + |
| +**STATUS:** |
| +**Request changes: two P2 findings.** |
| + |
| +1. **P2 — Inherited skipped entries still prevent successful scans.** The exemption at [publisher.go:73](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/hostfiles-cap-live-entries/internal/hostfiles/publisher.go:73) lets `Open` accept an over-cap skipped entry. After restore, that unpublished file is absent, so `syncRoot` calls `StageDelete`, which still returns `ErrTooDeep`. Consequently, `Publish` and `ReleaseFlushFiles.Flush` fail; `Run` repeatedly retries the scan. The new continuation test stages a healthy file directly, bypassing this failure. Handle deletion of an inherited skipped entry without allowing new over-cap live entries, and test restore → scan → commit. |
| + |
| +2. **P2 — Exempt tombstones reintroduce unbounded, non-cancellable restore work.** The relaxed validation at [restore.go:270](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/hostfiles-cap-live-entries/internal/hostfiles/restore.go:270) admits arbitrarily deep tombstones. Removal checks every ancestor with `realParents`; directory syncing repeats those checks for every ancestor again. On this Linux toolchain, `os.Root` resolves each prefix component by component, making the surviving-parent case cubic in depth, without context checks. This can occupy installation indefinitely instead of returning the previous immediate refusal. The new test uses an empty destination, so it never exercises existing deep parents or actual tombstone removal. Make traversal efficient and cancellable while preserving deletion durability. |
| + |
| +The shared validation path covers `OpenAt`, `CopyManifest`, `NewFromCopy`, and moves’ `Establish`; I found no new route for writing an over-cap live entry. `Present` skips exempt entries, though skipped paths still participate in `superseded`. The census envelope hash matches exactly; root-read units and the legacy seal are unchanged. |
| + |
| +Targeted `hostfiles` tests could not start: the read-only sandbox denied Go’s temporary build directory. The full `internal/host` suite was not run. |
| + |
| +**NEEDED:** |
| +Nothing from you for this review. |
| + |
| +**RECOMMENDED:** |
| +Fix both findings and add regressions before merging after the freeze. |
| + |
| +**SESSION:** |
| +Safe to close. HEAD and working tree are unchanged; no processes remain running. |
| \ No newline at end of file |
diff --git a/internal/hostfiles/fix4_linux_test.go b/internal/hostfiles/fix4_linux_test.go| index e093826ea..535311978 100644 |
| --- a/internal/hostfiles/fix4_linux_test.go |
| +++ b/internal/hostfiles/fix4_linux_test.go |
| @@ -77,8 +77,10 @@ func TestTheWritersRefuseAPathPastTheCapAsTheReadersDo(t *testing.T) { |
| if err := p.StageSkipped(past, "too-large"); !errors.Is(err, ErrTooDeep) { |
| t.Fatalf("StageSkipped past the cap: %v", err) |
| } |
| - if err := p.StageDelete(past); !errors.Is(err, ErrTooDeep) { |
| - t.Fatalf("StageDelete past the cap: %v", err) |
| + // StageDelete takes no cap: it only tombstones an entry the manifest |
| + // holds, and this one holds none, so it is a no-op. |
| + if err := p.StageDelete(past); err != nil || p.Dirty() && p.man.Entries[past].Deleted { |
| + t.Fatalf("StageDelete of a path the manifest never had: %v", err) |
| } |
| // A manifest the publisher continued that already holds such a live |
| // path (written before the cap) is refused at Commit, never written |
diff --git a/internal/hostfiles/fix5_linux_test.go b/internal/hostfiles/fix5_linux_test.go| index 56207dc7e..a6d414b52 100644 |
| --- a/internal/hostfiles/fix5_linux_test.go |
| +++ b/internal/hostfiles/fix5_linux_test.go |
| @@ -99,7 +99,9 @@ func TestAnInheritedManifestPastTheCapIsRefusedByOpenAndTheReaders(t *testing.T) |
| // Run over a publisher holding such an entry (one continued before Open |
| // refused them) stops on its first commit with one line, never retrying |
| -// at every tick, and commits nothing. |
| +// at every tick, and commits nothing. The scan would retire the entry if |
| +// its file were gone (StageDelete takes no cap), so the scan here skips |
| +// the path, as it skips one the caller excludes. |
| func TestRunStopsOnAManifestItCannotCommit(t *testing.T) { |
| logs := captureLog(t) |
| s, dir := release.NewDirStore(t.TempDir()), t.TempDir() |
| @@ -110,15 +112,18 @@ func TestRunStopsOnAManifestItCannotCommit(t *testing.T) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| + deep := pathOfBytes(MaxPathBytes + 1) |
| p.mu.Lock() |
| p.next++ |
| - p.man.Entries[pathOfBytes(MaxPathBytes+1)] = liveEntry(p.next) |
| + p.man.Entries[deep] = liveEntry(p.next) |
| p.mu.Unlock() |
| write(t, dir, "healthy.txt", "changed") // the first scan stages it, and Commit is attempted |
| ctx, cancel := context.WithCancel(context.Background()) |
| defer cancel() |
| + o := polled |
| + o.Skip = func(rel string, _ bool) bool { return rel == deep } |
| done := make(chan error, 1) |
| - go func() { done <- run(ctx, p, dir, polled, nil) }() |
| + go func() { done <- run(ctx, p, dir, o, nil) }() |
| select { |
| case err := <-done: |
| if !errors.Is(err, ErrTooDeep) { |
| @@ -184,3 +189,93 @@ func TestATombstoneOrSkippedEntryPastTheCapStaysReadable(t *testing.T) { |
| t.Fatalf("a live path past the cap: %v", err) |
| } |
| } |
| + |
| +// Astra's review of the exemption (P2 1): a skipped entry past the cap, |
| +// inherited, has no file after a restore, so the next scan retires it |
| +// (StageDelete), and the commit goes through, rather than every scan |
| +// failing on it. |
| +func TestAScanRetiresAnInheritedSkippedEntryPastTheCap(t *testing.T) { |
| + ctx := context.Background() |
| + s, dir := release.NewDirStore(t.TempDir()), t.TempDir() |
| + skipped := pathOfBytes(MaxPathBytes + 1) |
| + inheritPastTheCap(t, s, func(seq int64) map[string]Entry { |
| + return map[string]Entry{skipped: {Seq: seq, Skipped: "too-large"}} |
| + }) |
| + if _, _, err := Restore(ctx, s, pfx, dir, nil); err != nil { |
| + t.Fatalf("Restore: %v", err) |
| + } |
| + p, err := Open(ctx, s, pfx) |
| + if err != nil { |
| + t.Fatalf("Open: %v", err) |
| + } |
| + if _, err := SyncDir(ctx, p, dir, nil); err != nil { |
| + t.Fatalf("scan after the restore: %v", err) |
| + } |
| + commit(t, p) |
| + m, _, err := Load(ctx, s, pfx) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if !m.Entries[skipped].Deleted || m.Entries["healthy.txt"].Deleted { |
| + t.Fatalf("after the scan: skipped %+v, healthy %+v", m.Entries[skipped], m.Entries["healthy.txt"]) |
| + } |
| +} |
| + |
| +// Astra's review of the exemption (P2 2): Restore never walks a tombstone |
| +// past the cap, so a deep tree standing at the restore's destination |
| +// costs it nothing and is left where it is (the next walk refuses it). |
| +func TestRestoreNeverWalksATombstonePastTheCap(t *testing.T) { |
| + ctx := context.Background() |
| + s, dir := release.NewDirStore(t.TempDir()), t.TempDir() |
| + // A file deeper than the cap, built one directory at a time. |
| + r, err := os.OpenRoot(dir) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var parts []string |
| + for n := 0; n < MaxPathBytes/2+1; n++ { |
| + if err := r.Mkdir("d", 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + next, err := r.OpenRoot("d") |
| + r.Close() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + r, parts = next, append(parts, "d") |
| + } |
| + if err := r.WriteFile("f", []byte("deep"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + r.Close() |
| + deep := strings.Join(append(parts, "f"), "/") |
| + if withinTreeCap(deep) == nil { |
| + t.Fatalf("the fixture is within the cap: %d bytes", len(deep)) |
| + } |
| + inheritPastTheCap(t, s, func(seq int64) map[string]Entry { |
| + return map[string]Entry{deep: {Seq: seq, Deleted: true}} |
| + }) |
| + _, st, err := Restore(ctx, s, pfx, dir, nil) |
| + if err != nil { |
| + t.Fatalf("Restore: %v", err) |
| + } |
| + if st.Removed != 0 || st.Written != 1 { |
| + t.Fatalf("restore: %+v", st) |
| + } |
| + r, err = os.OpenRoot(dir) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, d := range parts { |
| + next, err := r.OpenRoot(d) |
| + r.Close() |
| + if err != nil { |
| + t.Fatalf("the deep tree was touched: %v", err) |
| + } |
| + r = next |
| + } |
| + defer r.Close() |
| + if b, err := r.ReadFile("f"); err != nil || string(b) != "deep" { |
| + t.Fatalf("the deep file: %q %v", b, err) |
| + } |
| +} |
diff --git a/internal/hostfiles/publisher.go b/internal/hostfiles/publisher.go| index 17cc6e834..5fc225dcf 100644 |
| --- a/internal/hostfiles/publisher.go |
| +++ b/internal/hostfiles/publisher.go |
| @@ -174,15 +174,16 @@ func (p *Publisher) StageSkipped(path, why string) error { |
| } |
| // StageDelete records that path is gone. Deleting a path the manifest never |
| -// had, or already has as deleted, is a no-op. |
| +// had, or already has as deleted, is a no-op. It takes no length cap: it |
| +// only tombstones an entry the manifest already holds, and a tombstone is |
| +// exempt (LiveEntriesWithinCap), so a skipped entry past the cap that a |
| +// manifest written before the cap carried can still be retired when its |
| +// file is gone. |
| func (p *Publisher) StageDelete(path string) error { |
| clean, err := CleanPath(path) |
| if err != nil { |
| return err |
| } |
| - if err := withinTreeCap(clean); err != nil { |
| - return err |
| - } |
| p.mu.Lock() |
| defer p.mu.Unlock() |
| if cur, ok := p.man.Entries[clean]; !ok || cur.Deleted { |
diff --git a/internal/hostfiles/restore.go b/internal/hostfiles/restore.go| index 3fc5af304..9dcb18d3c 100644 |
| --- a/internal/hostfiles/restore.go |
| +++ b/internal/hostfiles/restore.go |
| @@ -106,9 +106,16 @@ func restore(ctx context.Context, s release.Store, prefix string, origins []stri |
| } |
| var live, dead []string |
| for p, e := range m.Entries { |
| - if e.Deleted { |
| + switch { |
| + case e.Deleted && withinTreeCap(p) != nil: |
| + // A tombstone past the cap (one a manifest written before the cap |
| + // may hold) is never walked: removing it would resolve every |
| + // component of a path no restore could have written, without |
| + // bound. Anything still standing there is left to the next walk, |
| + // which refuses a path past the cap (ErrTooDeep). |
| + case e.Deleted: |
| dead = append(dead, p) |
| - } else { |
| + default: |
| live = append(live, p) |
| } |
| } |
diff --git a/internal/hostfiles/sync.go b/internal/hostfiles/sync.go| index 65158cf8b..23e7edffb 100644 |
| --- a/internal/hostfiles/sync.go |
| +++ b/internal/hostfiles/sync.go |
| @@ -76,10 +76,13 @@ func SyncDir(ctx context.Context, p *Publisher, dir string, skip func(rel string |
| // MaxPathBytes caps a path in a files tree, relative to its root: PATH_MAX |
| // (4096) less its terminating NUL, as moves' archive contract has it. It is |
| -// the one cap every writer (Stage, StageSkipped, StageDelete, Commit, the |
| -// watcher's publishOne, a walk) and every reader (checkManifest: Restore, |
| -// CopyManifest, ValidateManifest) applies, so no manifest is written that |
| -// a reader refuses. Every path it admits can be opened by one openat2 |
| +// the one cap every writer of a new entry (Stage, StageSkipped, the |
| +// watcher's publishOne, a walk) applies, and that Commit and every reader |
| +// (checkManifest: Restore, CopyManifest, ValidateManifest) apply to live |
| +// entries (LiveEntriesWithinCap), so no manifest is written that a reader |
| +// refuses. A tombstone or skipped entry past it, from a manifest written |
| +// before the cap, is exempt: StageDelete retires one, and Restore neither |
| +// writes nor walks it. Every path it admits can be opened by one openat2 |
| // beneath the restored root (Present). It also bounds depth (about 2048 |
| // components of one byte), and with it a walk's open directories and its |
| // path strings. |
diff --git a/internal/provision/testdata/plan14-root-reads-hostfiles-cap-236.tsv b/internal/provision/testdata/plan14-root-reads-hostfiles-cap-236.tsv| index 1e1c1c83a..f115c9564 100644 |
| --- a/internal/provision/testdata/plan14-root-reads-hostfiles-cap-236.tsv |
| +++ b/internal/provision/testdata/plan14-root-reads-hostfiles-cap-236.tsv |
| @@ -1,2 +1,2 @@ |
| -# Plan 14 census overlay hostfiles-cap-236 (amadan #236, owner region HA, basis moves' review of a8cebad): the hostfiles path cap applies to live entries only (LiveEntriesWithinCap). Tombstones and skipped entries past the cap are exempt, since Restore writes neither, so a manifest written before the cap stays readable and a moves recovery that selects it is not stranded. manifestWithinCap's body and checkManifest change; Open, OpenAt and Commit are byte-identical. Envelope-only: no root-read unit changed or was added, nothing runs as root that did not before, and no registry, /data, release or claim read was added, so the legacy seal stands. |
| -@coverage internal/hostfiles 1b7630f1bd066784e51a5b9cbb4d300a4c590fbaae4b08d43013373e9d50c65e |
| +# Plan 14 census overlay hostfiles-cap-236 (amadan #236, owner region HA, basis moves' review of a8cebad): the hostfiles path cap applies to live entries only (LiveEntriesWithinCap). Tombstones and skipped entries past the cap are exempt, since Restore writes neither, so a manifest written before the cap stays readable and a moves recovery that selects it is not stranded. manifestWithinCap's body, checkManifest, Restore's partition (a tombstone past the cap is never walked) and StageDelete (no cap: it only tombstones an entry the manifest holds) change; Open, OpenAt and Commit are byte-identical. Envelope-only: no root-read unit changed or was added, nothing runs as root that did not before, and no registry, /data, release or claim read was added, so the legacy seal stands. |
| +@coverage internal/hostfiles 785345dc33d232e31bde402a79708cb0e74293cf209eb22e6c1dda646bb71293 |