From 4077665ae150c482620d37250b774b23a2e3b771 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 10:59:42 -0400 Subject: [PATCH 1/7] fix(bundle): undo new members after reconcile refusal (D-0760) --- .github/wiki/Bundles.md | 2 + internal/bundle/installer_appready.go | 13 +- internal/bundle/installer_appready_undo.go | 91 ++++++++++ .../bundle/installer_appready_undo_test.go | 155 ++++++++++++++++++ 4 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 internal/bundle/installer_appready_undo.go create mode 100644 internal/bundle/installer_appready_undo_test.go diff --git a/.github/wiki/Bundles.md b/.github/wiki/Bundles.md index e7bec92c9..4f9f86999 100644 --- a/.github/wiki/Bundles.md +++ b/.github/wiki/Bundles.md @@ -20,6 +20,8 @@ How `nself bundle install` reaches an app-ready stack, and how nself records whi The first failing step stops the run; later steps show `skipped`. Outside an nSelf project the plugins are installed and recorded, the later steps are skipped, and the output says `NOT VERIFIED`; the exit code stays 0. +If reconcile fails or is refused (including E403, E480 and E481), the install removes the members it newly added and their ledger records, and restores prior bundle ownership. Existing members and their schemas stay intact, including schemas kept by an earlier removal. Cleanup errors are returned alongside the reconcile error; a member that could not be removed keeps its ledger record. A failure in a later app-ready step keeps the installed members for a retry. + `nself bundle install --dry-run` prints the nine steps as `planned` and changes nothing: no files, no ledger, no hooks. A bundle that is not app-ready fails with **E135** (exit 2) naming the bundle, the step and the plugin. Starting the stack and the readiness, tracking and health steps are `NSELF_V15=1` behaviour. With `NSELF_V15` off the command installs, records the ledger and reconciles (a failed reconcile is an error in both modes), prints those four steps as `skipped` / `not verified (v1.5 behaviour, NSELF_V15=1)`, ends with `NOT VERIFIED` instead of `APP-READY`, and exits 0. A free bundle skips the ping_api bundle entitlement call; each plugin's licence is still checked. Rerun the command after fixing the cause; members that are already installed are not reinstalled. diff --git a/internal/bundle/installer_appready.go b/internal/bundle/installer_appready.go index 6eec06568..a82ed1f38 100644 --- a/internal/bundle/installer_appready.go +++ b/internal/bundle/installer_appready.go @@ -10,10 +10,12 @@ package bundle // the plugins that were on disk before the install (name -> version). // Outputs: nil, or E135 (v1.5) / a printed warning (v1.4) for a failed step. // Constraints: a ledger write failure does NOT roll the install back (the plugins -// are on disk); the error says so. Hooks nil: only the ledger step runs. +// are on disk); the error says so. A failed reconcile undoes fresh members and +// restores ledger ownership. Hooks nil: only the ledger step runs. import ( "context" + "errors" "fmt" "io" "os" @@ -73,6 +75,7 @@ func finishInstall(ctx context.Context, rn *appready.Runner, store *ledger.Store out = os.Stderr } var names []string + var prior ledger.Ledger _ = rn.Step(appready.StepLedger, func() (string, error) { members, err := installedMembers(pluginDir, planned) if err != nil { @@ -82,6 +85,7 @@ func finishInstall(ctx context.Context, rn *appready.Runner, store *ledger.Store return "no installed members to record", nil } if err := store.Locked(func(tx *ledger.Tx, l ledger.Ledger) error { + prior = l.Clone() // A member that was on disk before this install and has no ledger // record was installed on its own: record it explicit so a later // bundle remove keeps it. (Errs toward keeping the plugin.) @@ -123,7 +127,12 @@ func finishInstall(ctx context.Context, rn *appready.Runner, store *ledger.Store appready.Render(out, rep) } if f := rn.Failure(); f != nil { - return appReadyVerdict(out, result.Bundle, f) + verdict := appReadyVerdict(out, result.Bundle, f) + if f.Step == appready.StepReconcile { + undoErr := undoFreshMembers(ctx, store, pluginDir, planned, prior, result, before, out) + return errors.Join(verdict, undoErr) + } + return verdict } return nil } diff --git a/internal/bundle/installer_appready_undo.go b/internal/bundle/installer_appready_undo.go new file mode 100644 index 000000000..89253625e --- /dev/null +++ b/internal/bundle/installer_appready_undo.go @@ -0,0 +1,91 @@ +package bundle + +// installer_appready_undo.go — undo a bundle install whose reconcile failed. +// +// Purpose: a refused or failed reconcile (E403 without --yes, E480/E481, a +// build error) must not leave the members this install added on disk and in the +// ledger, or later builds would see them (D-0760, same class as D-0433). +// Inputs: the planned members, the plugins that were on disk before the install +// (name -> version) and the ledger before recording this install. +// Outputs: the fresh members removed again (files and command binary) and their +// ledger records dropped; the reconcile error stays the caller's error. +// Constraints: only members absent before the install are undone; a plugin that +// was already on disk (same version, kept higher version or an in-place upgrade) +// stays installed. The undo never drops a schema: an install reuses a schema +// kept by an earlier remove, so plugin.UndoInstall leaves it alone. + +import ( + "context" + "errors" + "fmt" + "io" + "maps" + "strings" + + "github.com/nself-org/cli/internal/bundle/ledger" + "github.com/nself-org/cli/internal/plugin" +) + +// freshMembers returns the planned members that were not on disk before the install. +func freshMembers(planned []string, before map[string]string) []string { + var fresh []string + for _, name := range planned { + if _, there := before[strings.ToLower(name)]; !there { + fresh = append(fresh, name) + } + } + return fresh +} + +// undoFreshMembers removes the fresh members from disk (in reverse install +// order) and from the ledger, and returns the first-line errors joined. A +// member that could not be undone keeps its ledger record so disk and ledger +// agree. Prior ownership is restored for existing members; an incomplete undo +// keeps a newly recorded bundle so its remaining members can still be removed. +func undoFreshMembers(ctx context.Context, store *ledger.Store, pluginDir string, planned []string, prior ledger.Ledger, result *InstallResult, before map[string]string, out io.Writer) error { + fresh := freshMembers(planned, before) + var errsOut []error + var gone []string + for i := len(fresh) - 1; i >= 0; i-- { + name := fresh[i] + _, _ = fmt.Fprintf(out, " ↩ rolling back %s (the build did not apply)...\n", name) + if err := plugin.UndoInstall(ctx, nil, name, pluginDir); err != nil { + errsOut = append(errsOut, fmt.Errorf("undoing the install of plugin %q: %w", name, err)) + continue + } + gone = append(gone, name) + } + result.RolledBack = append(result.RolledBack, gone...) + if err := store.Update(func(l *ledger.Ledger) error { + restoreInstallOwnership(l, prior, result.Bundle, planned, before, gone, len(gone) == len(fresh)) + return nil + }); err != nil { + errsOut = append(errsOut, fmt.Errorf("the plugins are removed but updating %s failed: %w", store.Path(), err)) + } + result.Changed = !maps.Equal(before, buildInstalledVersionMap(pluginDir)) + return errors.Join(errsOut...) +} + +// restoreInstallOwnership restores prior references while retaining facts for +// upgraded members and the records of fresh members whose undo failed. +func restoreInstallOwnership(l *ledger.Ledger, prior ledger.Ledger, slug string, planned []string, before map[string]string, gone []string, complete bool) { + for _, name := range planned { + key := strings.ToLower(name) + if _, existed := before[key]; !existed { + continue + } + rec := l.Plugins[key] + old, recorded := prior.Plugins[key] + rec.InstalledBy = old.InstalledBy + rec.Explicit = old.Explicit || !recorded + l.Plugins[key] = rec + } + for _, name := range gone { + delete(l.Plugins, strings.ToLower(name)) + } + if old, recorded := prior.Bundles[slug]; recorded { + l.Bundles[slug] = old + } else if complete { + delete(l.Bundles, slug) + } +} diff --git a/internal/bundle/installer_appready_undo_test.go b/internal/bundle/installer_appready_undo_test.go new file mode 100644 index 000000000..88a8219b1 --- /dev/null +++ b/internal/bundle/installer_appready_undo_test.go @@ -0,0 +1,155 @@ +package bundle + +// installer_appready_undo_test.go — D-0760: a failed or refused reconcile after a +// bundle install undoes the members that install added and their ledger records. + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "github.com/nself-org/cli/internal/bundle/appready" + "github.com/nself-org/cli/internal/bundle/ledger" +) + +var errRefused = errors.New("E403: the build needs --yes") + +func refusedHooks(t *testing.T) *appready.Hooks { + t.Helper() + return &appready.Hooks{ + Reconcile: func(context.Context) error { return errRefused }, + Start: func(context.Context) error { t.Error("start ran after a refused reconcile"); return nil }, + } +} + +// TestBundleInstallRefusedReconcileUndoesMembers: every fresh member dir and the +// bundle's ledger records are gone after the reconcile is refused; the error is +// still the reconcile error. +func TestBundleInstallRefusedReconcileUndoesMembers(t *testing.T) { + for _, code := range []string{"E403", "E480", "E481"} { + t.Run(code, func(t *testing.T) { + pluginDir := isolate(t) + setupOfflineRegistry(t, mockTaskRegistry()) + var out bytes.Buffer + opts := freeOpts(t, pluginDir, &out, func(name string) error { + writeMember(t, pluginDir, name, 1, []string{"np_kept_data"}) + return nil + }) + opts.Hooks = refusedHooks(t) + cause := errors.New(code + ": reconcile refused") + opts.Hooks.Reconcile = func(context.Context) error { return cause } + + res, err := Install(context.Background(), "task", opts) + if err == nil || !errors.Is(err, cause) || !strings.Contains(err.Error(), "reconcile failed after install") { + t.Fatalf("err = %v, want the reconcile error", err) + } + for _, name := range []string{"notifications", "jobs"} { + if _, statErr := os.Stat(filepath.Join(pluginDir, name)); !errors.Is(statErr, os.ErrNotExist) { + t.Errorf("member %q still on disk after a refused reconcile (stat err %v)", name, statErr) + } + } + l, _, lerr := ledger.NewStore(pluginDir).Load() + if lerr != nil || len(l.Bundles) != 0 || len(l.Plugins) != 0 { + t.Errorf("ledger after the undo: err=%v bundles=%v plugins=%v", lerr, l.Bundles, l.Plugins) + } + if len(res.RolledBack) != 2 || res.Changed { + t.Errorf("RolledBack=%v Changed=%v, want both members rolled back and no change", res.RolledBack, res.Changed) + } + }) + } +} + +// A refused upgrade has no fresh members to remove, but must still undo newly +// recorded ownership while preserving prior bundle associations. +func TestBundleInstallRefusedUpgradeRestoresOwnership(t *testing.T) { + for _, recorded := range []bool{false, true} { + t.Run(fmt.Sprint(recorded), func(t *testing.T) { + pluginDir := isolate(t) + setupOfflineRegistry(t, mockTaskRegistry()) + for _, name := range []string{"notifications", "jobs"} { + writeMember(t, pluginDir, name, 1, nil) + path := filepath.Join(pluginDir, name, "plugin.json") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, bytes.ReplaceAll(raw, []byte("1.0.0"), []byte("0.9.0")), 0o644); err != nil { + t.Fatal(err) + } + } + store := newLedgerStore(pluginDir) + if err := store.Update(func(l *ledger.Ledger) error { + members := []ledger.InstalledPlugin{{Name: "jobs", Version: "0.9.0"}, {Name: "notifications", Version: "0.9.0"}} + l.AddBundle("existing", members, time.Now()) + if recorded { + l.AddBundle("task", members, time.Now()) + } + return nil + }); err != nil { + t.Fatal(err) + } + prior := loadLedger(t, pluginDir) + var out bytes.Buffer + opts := freeOpts(t, pluginDir, &out, func(name string) error { writeMember(t, pluginDir, name, 1, nil); return nil }) + opts.Hooks = refusedHooks(t) + if _, err := Install(context.Background(), "task", opts); !errors.Is(err, errRefused) { + t.Fatalf("install: %v", err) + } + after := loadLedger(t, pluginDir) + if !reflect.DeepEqual(after.Bundles, prior.Bundles) { + t.Errorf("bundles = %v, want %v", after.Bundles, prior.Bundles) + } + for name, old := range prior.Plugins { + got := after.Plugins[name] + if !reflect.DeepEqual(got.InstalledBy, old.InstalledBy) || got.Explicit != old.Explicit { + t.Errorf("%s ownership = %+v, want %+v", name, got, old) + } + if _, err := os.Stat(filepath.Join(pluginDir, name)); err != nil { + t.Errorf("existing member removed: %v", err) + } + } + }) + } +} + +// TestBundleInstallRefusedReconcileKeepsPreexistingMember: a member that was on +// disk before the install stays installed and recorded as explicit; only the +// member this install added goes. +func TestBundleInstallRefusedReconcileKeepsPreexistingMember(t *testing.T) { + pluginDir := isolate(t) + setupOfflineRegistry(t, mockTaskRegistry()) + writeMember(t, pluginDir, "jobs", 1, nil) + var out bytes.Buffer + opts := freeOpts(t, pluginDir, &out, func(name string) error { + writeMember(t, pluginDir, name, 1, nil) + return nil + }) + opts.Hooks = refusedHooks(t) + + if _, err := Install(context.Background(), "task", opts); !errors.Is(err, errRefused) { + t.Fatalf("err = %v, want the reconcile error", err) + } + if _, statErr := os.Stat(filepath.Join(pluginDir, "notifications")); !errors.Is(statErr, os.ErrNotExist) { + t.Errorf("fresh member notifications still on disk (stat err %v)", statErr) + } + if _, statErr := os.Stat(filepath.Join(pluginDir, "jobs")); statErr != nil { + t.Errorf("pre-existing member jobs was removed: %v", statErr) + } + l, _, lerr := ledger.NewStore(pluginDir).Load() + if lerr != nil || len(l.Bundles) != 0 { + t.Fatalf("ledger: err=%v bundles=%v, want no bundle record", lerr, l.Bundles) + } + if rec, ok := l.Plugins["jobs"]; !ok || !rec.Explicit || len(rec.InstalledBy) != 0 { + t.Errorf("jobs record = %+v (present %v), want explicit with no installed_by", rec, ok) + } + if _, ok := l.Plugins["notifications"]; ok { + t.Error("notifications is still in the ledger") + } +} From dd0f40926e76781a0975c93679dd1bac703b510c Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 11:04:42 -0400 Subject: [PATCH 2/7] fix(build): confine proxy restore cleanup and plan links (D-1162) --- .github/wiki/Nginx-Generation.md | 5 + internal/build/expect_publish.go | 18 +++ internal/build/expect_rollback.go | 32 ++++- internal/build/expect_sink.go | 27 +++- internal/build/proxy_restore.go | 78 ++++++---- internal/build/proxy_restore_checked_test.go | 142 +++++++++++++++++++ internal/build/shared_proxy_sink.go | 5 + internal/build/sink.go | 15 +- 8 files changed, 283 insertions(+), 39 deletions(-) create mode 100644 internal/build/proxy_restore_checked_test.go diff --git a/.github/wiki/Nginx-Generation.md b/.github/wiki/Nginx-Generation.md index 31a84f18b..3f7fc2810 100644 --- a/.github/wiki/Nginx-Generation.md +++ b/.github/wiki/Nginx-Generation.md @@ -74,6 +74,11 @@ The add writes files through reconcile, then, in the same apply, removes the ngi ### Returning to nginx +Cleanup records dangling symlinks in the confirmed removal plan and unlinks +them without following their targets. Directory traversal and deletion stay +inside each generated output directory, including when a directory is replaced +by a symlink during cleanup or before the confirmed plan is published. + ```bash nself remove traefik --yes ``` diff --git a/internal/build/expect_publish.go b/internal/build/expect_publish.go index cf2c5b3a1..3737b94d4 100644 --- a/internal/build/expect_publish.go +++ b/internal/build/expect_publish.go @@ -92,6 +92,24 @@ func (p *artifactPublisher) target(path string) (*os.Root, string, error) { if err != nil { return nil, "", err } + for scope, expected := range p.sink.cleanupRoots { + if rel, inside := under(scope, abs); inside { + root := p.roots[scope] + if root == nil { + root, err = os.OpenRoot(scope) + if err != nil { + return nil, "", err + } + info, statErr := root.Stat(".") + if statErr != nil || !os.SameFile(expected, info) { + _ = root.Close() + return nil, "", errDeviated(scope, "cleanup directory was replaced") + } + p.roots[scope] = root + } + return root, filepath.FromSlash(rel), nil + } + } base := filepath.VolumeName(abs) + string(filepath.Separator) rel, inside := under(p.sink.root, abs) if inside { diff --git a/internal/build/expect_rollback.go b/internal/build/expect_rollback.go index 639798d80..d4a000e68 100644 --- a/internal/build/expect_rollback.go +++ b/internal/build/expect_rollback.go @@ -83,7 +83,10 @@ func (p *artifactPublisher) restore(before artifactBefore) error { if err := root.Mkdir(rel, before.info.Mode().Perm()); err != nil && !errors.Is(err, fs.ErrExist) { return err } - return root.Chmod(rel, before.info.Mode().Perm()) + if err := root.Chmod(rel, before.info.Mode().Perm()); err != nil { + return err + } + return p.refreshCleanupRoot(before.path, root, rel) } if before.link != "" { if err := root.Remove(rel); err != nil && !errors.Is(err, fs.ErrNotExist) { @@ -103,3 +106,30 @@ func (p *artifactPublisher) restore(before artifactBefore) error { } return p.write(before.path, data, before.info.Mode().Perm()) } + +// refreshCleanupRoot rebinds rollback's descendants to the directory rollback +// just recreated, rather than the cached descriptor for its unlinked predecessor. +func (p *artifactPublisher) refreshCleanupRoot(path string, parent *os.Root, rel string) error { + abs, err := filepath.Abs(p.sink.abs(path)) + if err != nil { + return err + } + if _, tracked := p.sink.cleanupRoots[abs]; !tracked { + return nil + } + root, err := parent.OpenRoot(rel) + if err != nil { + return err + } + info, err := root.Stat(".") + if err != nil { + _ = root.Close() + return err + } + if previous := p.roots[abs]; previous != nil { + _ = previous.Close() + } + p.roots[abs] = root + p.sink.cleanupRoots[abs] = info + return nil +} diff --git a/internal/build/expect_sink.go b/internal/build/expect_sink.go index 2b77bf752..2531e039e 100644 --- a/internal/build/expect_sink.go +++ b/internal/build/expect_sink.go @@ -42,12 +42,13 @@ func errDeviated(path, why string) error { // expectSink is the checking disk sink. type expectSink struct { *diskSink - exp *PlannedBuild - keyr *memSink // canonicalises paths exactly as plan mode did - dirs map[string]fs.FileMode - atomic map[string]bool - chmods map[string]fs.FileMode - writeModes map[string]fs.FileMode + exp *PlannedBuild + keyr *memSink // canonicalises paths exactly as plan mode did + dirs map[string]fs.FileMode + atomic map[string]bool + chmods map[string]fs.FileMode + writeModes map[string]fs.FileMode + cleanupRoots map[string]fs.FileInfo } func newExpectSink(root string, exp *PlannedBuild) *expectSink { @@ -104,6 +105,20 @@ func (e *expectSink) Remove(p string) error { return e.keyr.Remove(p) } +// RemoveWithin stages an unlink and remembers the cleanup directory identity +// so deferred publication cannot be redirected to a different directory. +func (e *expectSink) RemoveWithin(root *os.Root, _ string, p string) error { + info, err := root.Stat(".") + if err != nil { + return err + } + if e.cleanupRoots == nil { + e.cleanupRoots = map[string]fs.FileInfo{} + } + e.cleanupRoots[root.Name()] = info + return e.Remove(p) +} + func (e *expectSink) Chmod(p string, perm fs.FileMode) error { k := e.keyr.key(p) if f, ok := e.exp.Files[k]; ok && f.Perm == perm { diff --git a/internal/build/proxy_restore.go b/internal/build/proxy_restore.go index 3564f49b3..6d0918107 100644 --- a/internal/build/proxy_restore.go +++ b/internal/build/proxy_restore.go @@ -20,6 +20,7 @@ import ( "log/slog" "os" "path/filepath" + "sort" ) // proxyGeneratedMarker is the file the traefik renderer writes beside its output; @@ -151,56 +152,75 @@ func proxyOwnedDir(workdir, dir string) (bool, string) { return true, "" } -// errTreeSwapped is returned when a directory being emptied stopped being the -// directory the walk found: something replaced it (for example with a symlink) -// between the check and the deletion. -var errTreeSwapped = errors.New("a directory was replaced while it was being removed") - -// stillSameDir reports whether dir is still the real directory identified by want. -// An Lstat never follows a final symlink, so a link swapped in for dir fails. -func stillSameDir(dir string, want fs.FileInfo) bool { - fi, err := os.Lstat(dir) - return err == nil && fi.IsDir() && os.SameFile(want, fi) -} - // removeTree deletes dir's files and then dir, through the sink. A symlink // entry is removed as a link, never followed. The marker goes last, so an // interrupted cleanup still leaves a directory the next build recognises. -// want is dir's Lstat from the caller's check: the walk re-checks it after the -// listing and before every removal, so a directory swapped for a symlink to -// somewhere else stops the cleanup instead of deleting through the link. -func removeTree(sink Sink, dir string, want fs.FileInfo) error { - entries, err := sink.ReadDir(dir) +func removeTree(sink Sink, dir string, before fs.FileInfo) error { + if !before.IsDir() { + return fmt.Errorf("cleanup directory %s was replaced", dir) + } + root, err := os.OpenRoot(dir) + if err != nil { + return err + } + defer func() { _ = root.Close() }() + after, err := root.Stat(".") if err != nil { return err } + if !os.SameFile(before, after) { + return fmt.Errorf("cleanup directory %s was replaced", dir) + } + if err := removeTreeIn(sink, root, dir, "."); err != nil { + return err + } + return sink.Remove(dir) +} + +// removeTreeIn anchors both traversal and disk unlinks to the generated output +// directory. A swapped symlink cannot redirect either operation to a sibling. +func removeTreeIn(sink Sink, root *os.Root, dir, relative string) error { + f, err := root.Open(relative) + if err != nil { + return err + } + entries, err := f.ReadDir(-1) + _ = f.Close() + if err != nil { + return err + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) for _, e := range entries { - p := filepath.Join(dir, e.Name()) + rel := filepath.Join(relative, e.Name()) + p := filepath.Join(dir, rel) if e.Name() == proxyGeneratedMarker { continue } - if !stillSameDir(dir, want) { - return fmt.Errorf("%s: %w", dir, errTreeSwapped) - } // The listing may be stale: only a directory that is still one is entered. - if fi, err := os.Lstat(p); e.IsDir() && err == nil && fi.IsDir() { - if err := removeTree(sink, p, fi); err != nil { + if fi, err := root.Lstat(rel); e.IsDir() && err == nil && fi.IsDir() { + if err := removeTreeIn(sink, root, dir, rel); err != nil { + return err + } + if err := sink.RemoveWithin(root, rel, p); err != nil { return err } continue } - // A dangling symlink reports not-exist through a stat-based sink. - if err := sink.Remove(p); err != nil && !errors.Is(err, fs.ErrNotExist) { + if err := sink.RemoveWithin(root, rel, p); err != nil && !errors.Is(err, fs.ErrNotExist) { return err } } - if !stillSameDir(dir, want) { - return fmt.Errorf("%s: %w", dir, errTreeSwapped) + marker := filepath.Join(relative, proxyGeneratedMarker) + if _, err := root.Lstat(marker); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err } - if err := sink.Remove(filepath.Join(dir, proxyGeneratedMarker)); err != nil && !errors.Is(err, fs.ErrNotExist) { + if err := sink.RemoveWithin(root, marker, filepath.Join(dir, marker)); err != nil && !errors.Is(err, fs.ErrNotExist) { return err } - return sink.Remove(dir) + return nil } // emptyPrecreatedDir reports whether dir is exactly /.nself/generated/, diff --git a/internal/build/proxy_restore_checked_test.go b/internal/build/proxy_restore_checked_test.go new file mode 100644 index 000000000..2988e6d0f --- /dev/null +++ b/internal/build/proxy_restore_checked_test.go @@ -0,0 +1,142 @@ +package build + +import ( + "errors" + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestProxyRestoreCheckedApplyRollback(t *testing.T) { + workdir, dir := restoreProject(t, testRendererMarker) + before := proxyHashes(t, workdir) + plan := newMemSink(workdir, "") + if err := cleanupProviderOutput(plan, workdir, ProviderNginx); err != nil { + t.Fatal(err) + } + apply := newExpectSink(workdir, plan.snapshot()) + if err := cleanupProviderOutput(apply, workdir, ProviderNginx); err != nil { + t.Fatal(err) + } + failed := errors.New("validation failed") + if err := apply.commit(func() error { return failed }); !errors.Is(err, failed) { + t.Fatalf("commit error: %v", err) + } + after := proxyHashes(t, workdir) + if len(after) != len(before) { + t.Fatalf("rollback changed output tree under %s: before=%v after=%v", dir, before, after) + } + for path, hash := range before { + if after[path] != hash { + t.Fatalf("rollback did not restore %s", path) + } + } +} + +type proxySwapSink struct { + Sink + t *testing.T + trigger, path, target string +} + +func (s *proxySwapSink) Remove(path string) error { + s.swap(path) + return s.Sink.Remove(path) +} + +func (s *proxySwapSink) RemoveWithin(root *os.Root, relative, path string) error { + s.swap(path) + return s.Sink.RemoveWithin(root, relative, path) +} + +func (s *proxySwapSink) swap(path string) { + if path == s.trigger { + if err := os.Rename(s.path, s.path+"-original"); err != nil { + s.t.Fatal(err) + } + if err := os.Symlink(s.target, s.path); err != nil { + s.t.Fatal(err) + } + s.trigger = "" + } +} + +func TestProxyRestoreDirectorySwapKeepsSibling(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on windows") + } + workdir, dir := restoreProject(t, testRendererMarker) + outside := filepath.Join(workdir, "unrelated") + keep := filepath.Join(outside, "static.yml") + writeFixtureFile(t, keep, "keep me\n", 0644) + sink := &proxySwapSink{Sink: newDiskSink(workdir), t: t, + trigger: filepath.Join(dir, "sub", "static.yml"), path: filepath.Join(dir, "sub"), target: outside} + if err := cleanupProviderOutput(sink, workdir, ProviderNginx); err == nil { + t.Fatal("cleanup accepted a directory swapped outside the generated tree") + } + data, err := os.ReadFile(keep) + if err != nil || string(data) != "keep me\n" { + t.Fatalf("cleanup changed unrelated project file: %q, %v", data, err) + } +} + +func TestProxyRestoreCheckedApplyDirectorySwapKeepsSibling(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on windows") + } + workdir, dir := restoreProject(t, testRendererMarker) + outside := filepath.Join(workdir, "unrelated") + keep := filepath.Join(outside, "static.yml") + writeFixtureFile(t, keep, "keep me\n", 0644) + plan := newMemSink(workdir, "") + if err := cleanupProviderOutput(plan, workdir, ProviderNginx); err != nil { + t.Fatal(err) + } + apply := newExpectSink(workdir, plan.snapshot()) + if err := cleanupProviderOutput(apply, workdir, ProviderNginx); err != nil { + t.Fatal(err) + } + sub := filepath.Join(dir, "sub") + if err := os.Rename(sub, sub+"-original"); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, sub); err != nil { + t.Fatal(err) + } + if err := apply.commit(); err == nil { + t.Fatal("checked apply accepted a symlink escaping the cleanup directory") + } + data, err := os.ReadFile(keep) + if err != nil || string(data) != "keep me\n" { + t.Fatalf("checked apply changed unrelated project file: %q, %v", data, err) + } +} + +func TestProxyRestoreDanglingLinkCheckedApply(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on windows") + } + workdir, dir := restoreProject(t, testRendererMarker) + link := filepath.Join(dir, "dangling") + if err := os.Symlink(filepath.Join(workdir, "gone"), link); err != nil { + t.Fatal(err) + } + plan := newMemSink(workdir, "") + if err := cleanupProviderOutput(plan, workdir, ProviderNginx); err != nil { + t.Fatal(err) + } + if !plan.removed[plan.key(link)] { + t.Fatal("plan omitted dangling symlink removal") + } + apply := newExpectSink(workdir, plan.snapshot()) + if err := cleanupProviderOutput(apply, workdir, ProviderNginx); err != nil { + t.Fatal(err) + } + if err := apply.commit(); err != nil { + t.Fatal(err) + } + if exists(dir) { + t.Fatal("checked apply left generated output behind") + } +} diff --git a/internal/build/shared_proxy_sink.go b/internal/build/shared_proxy_sink.go index 757c002bb..6d3ee62fd 100644 --- a/internal/build/shared_proxy_sink.go +++ b/internal/build/shared_proxy_sink.go @@ -18,6 +18,7 @@ package build import ( "io/fs" + "os" "path/filepath" "strings" @@ -107,6 +108,10 @@ func (w *sharedSink) Remove(p string) error { return w.Sink.Remove(p) } +func (w *sharedSink) RemoveWithin(root *os.Root, relative, p string) error { + return w.Sink.RemoveWithin(root, relative, p) +} + func (w *sharedSink) ReadDir(p string) ([]fs.DirEntry, error) { entries, err := w.Sink.ReadDir(p) if err != nil || !w.inSites(filepath.Join(p, "x")) { diff --git a/internal/build/sink.go b/internal/build/sink.go index 1f8e44c5c..8b3aecefd 100644 --- a/internal/build/sink.go +++ b/internal/build/sink.go @@ -48,6 +48,8 @@ type Sink interface { // WriteAtomic writes via temp file + rename; the final mode is exactly perm. WriteAtomic(path string, data []byte, perm fs.FileMode) error Remove(path string) error + // RemoveWithin confines an unlink to an already opened cleanup directory. + RemoveWithin(root *os.Root, relative, path string) error Chmod(path string, perm fs.FileMode) error MkdirAll(path string, perm fs.FileMode) error ReadFile(path string) ([]byte, error) @@ -69,7 +71,10 @@ func (d *diskSink) WriteFile(p string, data []byte, perm fs.FileMode) error { func (d *diskSink) WriteAtomic(p string, data []byte, perm fs.FileMode) error { return atomicWrite(d.abs(p), data, perm) } -func (d *diskSink) Remove(p string) error { return os.Remove(d.abs(p)) } +func (d *diskSink) Remove(p string) error { return os.Remove(d.abs(p)) } +func (d *diskSink) RemoveWithin(root *os.Root, relative, _ string) error { + return root.Remove(relative) +} func (d *diskSink) Chmod(p string, perm fs.FileMode) error { return os.Chmod(d.abs(p), perm) } func (d *diskSink) MkdirAll(p string, perm fs.FileMode) error { return os.MkdirAll(d.abs(p), perm) } func (d *diskSink) ReadFile(p string) ([]byte, error) { return os.ReadFile(d.abs(p)) } @@ -158,7 +163,7 @@ func (m *memSink) Remove(p string) error { if _, planned := m.files[k]; planned { delete(m.files, k) // Removing a planned file also hides an older on-disk copy, if any. - if _, err := os.Stat(m.abs(p)); err == nil { + if _, err := os.Lstat(m.abs(p)); err == nil { m.removed[k] = true } return nil @@ -166,13 +171,17 @@ func (m *memSink) Remove(p string) error { if m.removed[k] { return &fs.PathError{Op: "remove", Path: p, Err: fs.ErrNotExist} } - if _, err := os.Stat(m.abs(p)); err != nil { + if _, err := os.Lstat(m.abs(p)); err != nil { return err } m.removed[k] = true return nil } +func (m *memSink) RemoveWithin(_ *os.Root, _ string, p string) error { + return m.Remove(p) +} + func (m *memSink) Chmod(p string, perm fs.FileMode) error { k := m.key(p) if f, ok := m.files[k]; ok { From 45ff8b3d992a8f6751a4ce434d5b53125ba2dfe6 Mon Sep 17 00:00:00 2001 From: admin Date: Sun, 25 Oct 2026 11:27:57 -0400 Subject: [PATCH 3/7] test(bundle): make the refused-upgrade ownership restore test fail without the fix (D-0760) Persist the ledger before the members exist so Store.Update does not bootstrap a task record from disk, and assert the prior installed_by before the install. --- .../bundle/installer_appready_undo_test.go | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/bundle/installer_appready_undo_test.go b/internal/bundle/installer_appready_undo_test.go index 88a8219b1..907958a10 100644 --- a/internal/bundle/installer_appready_undo_test.go +++ b/internal/bundle/installer_appready_undo_test.go @@ -73,6 +73,13 @@ func TestBundleInstallRefusedUpgradeRestoresOwnership(t *testing.T) { t.Run(fmt.Sprint(recorded), func(t *testing.T) { pluginDir := isolate(t) setupOfflineRegistry(t, mockTaskRegistry()) + // Persist an empty ledger before any member exists: otherwise the + // first Update bootstraps from the members on disk and infers a + // "task" record, which hides a missing ownership restore. + store := newLedgerStore(pluginDir) + if err := store.Update(func(*ledger.Ledger) error { return nil }); err != nil { + t.Fatal(err) + } for _, name := range []string{"notifications", "jobs"} { writeMember(t, pluginDir, name, 1, nil) path := filepath.Join(pluginDir, name, "plugin.json") @@ -84,7 +91,6 @@ func TestBundleInstallRefusedUpgradeRestoresOwnership(t *testing.T) { t.Fatal(err) } } - store := newLedgerStore(pluginDir) if err := store.Update(func(l *ledger.Ledger) error { members := []ledger.InstalledPlugin{{Name: "jobs", Version: "0.9.0"}, {Name: "notifications", Version: "0.9.0"}} l.AddBundle("existing", members, time.Now()) @@ -96,6 +102,19 @@ func TestBundleInstallRefusedUpgradeRestoresOwnership(t *testing.T) { t.Fatal(err) } prior := loadLedger(t, pluginDir) + wantBy := []string{"existing"} + if recorded { + wantBy = []string{"existing", "task"} + } + _, hasTask := prior.Bundles["task"] + if hasTask != recorded { + t.Fatalf("prior task bundle recorded = %v, want %v", hasTask, recorded) + } + for _, name := range []string{"jobs", "notifications"} { + if got := prior.Plugins[name].InstalledBy; !reflect.DeepEqual(got, wantBy) { + t.Fatalf("prior %s installed_by = %v, want %v", name, got, wantBy) + } + } var out bytes.Buffer opts := freeOpts(t, pluginDir, &out, func(name string) error { writeMember(t, pluginDir, name, 1, nil); return nil }) opts.Hooks = refusedHooks(t) From 1240811172f1b46a4cd077ad6e5edec327103ee4 Mon Sep 17 00:00:00 2001 From: admin Date: Sun, 25 Oct 2026 15:00:56 -0400 Subject: [PATCH 4/7] fix(bundle): keep installed members when reconcile fails after files were committed (D-0760) --- cmd/commands/bundle_reconcile.go | 7 ++- cmd/commands/bundle_reconcile_orphans_test.go | 53 +++++++++++++++++++ internal/bundle/appready/hooks.go | 21 ++++++++ internal/bundle/installer_appready.go | 4 +- .../bundle/installer_appready_undo_test.go | 33 ++++++++++++ 5 files changed, 115 insertions(+), 3 deletions(-) diff --git a/cmd/commands/bundle_reconcile.go b/cmd/commands/bundle_reconcile.go index 3a348d017..668d356a1 100644 --- a/cmd/commands/bundle_reconcile.go +++ b/cmd/commands/bundle_reconcile.go @@ -13,6 +13,7 @@ import ( "os" "github.com/nself-org/cli/internal/build" + "github.com/nself-org/cli/internal/bundle/appready" "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/docker" "github.com/nself-org/cli/internal/errs" @@ -68,13 +69,15 @@ func reconcileAfterExtension(cmd *cobra.Command, trigger reconcile.Trigger) erro } return fmt.Errorf("regenerating %s:%s: %w", trigger.Kind, trigger.Subject, err) } + // Apply wrote the generated files: from here a failure is post-commit, and + // a bundle install must keep its plugins (the files reference them). if err := swapContainers(context.Background(), cmd, projectDir, plan); err != nil { - return err + return appready.Committed(err) } if plan.RequiresConfirmation || len(reconcile.HandEditedPaths(*plan)) > 0 { return nil // Apply already printed the plan before its confirmation gate. } - return reconcile.RenderHuman(cmd.ErrOrStderr(), *plan) + return appready.Committed(reconcile.RenderHuman(cmd.ErrOrStderr(), *plan)) } // Seams for the container half of an extension reconcile; tests replace them. diff --git a/cmd/commands/bundle_reconcile_orphans_test.go b/cmd/commands/bundle_reconcile_orphans_test.go index cdde6044e..9a5c8dfc2 100644 --- a/cmd/commands/bundle_reconcile_orphans_test.go +++ b/cmd/commands/bundle_reconcile_orphans_test.go @@ -7,12 +7,14 @@ package commands import ( "context" + "errors" "os" "path/filepath" "reflect" "strings" "testing" + "github.com/nself-org/cli/internal/bundle/appready" "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/compat/compattest" "github.com/nself-org/cli/internal/docker" @@ -148,3 +150,54 @@ func TestExtensionComposeUsesProjectManifest(t *testing.T) { t.Fatalf("env files must follow the project: %v", c.EnvFiles) } } + +// A failed container swap happens after Apply wrote the generated files, so the +// error must say so (appready.ErrCommitted): the bundle install keeps its +// plugins then. A refused apply wrote nothing and must not carry the mark. +func TestExtensionReconcileSwapFailureIsCommitted(t *testing.T) { + trigger := reconcile.Trigger{Kind: reconcile.TriggerBundle, Subject: "zzfixture"} + running := fakeSwapRuntime{running: []reconcile.RunningContainer{ + {Name: "proj_oldproxy", Service: "oldproxy", State: "running", ConfigHash: "x"}, + }} + cases := map[string]func(){ + "remove": func() { + extensionRemoveOrphans = func(context.Context, []docker.OrphanContainer) []error { + return []error{errors.New("docker rm failed")} + } + }, + "start": func() { + extensionStartServices = func(context.Context, string, []string) error { return errors.New("compose up failed") } + }, + } + for name, fail := range cases { + t.Run(name, func(t *testing.T) { + compattest.Both(t, func(t *testing.T) { + if !compat.V15() { + return + } + extensionFixture(t, "") + localPluginRegistry(t) + swapSeams(t, running) + fail() + err := reconcileAfterExtension(pluginInstallCmd, trigger) + if err == nil || !errors.Is(err, appready.ErrCommitted) { + t.Fatalf("err = %v, want a post-commit failure", err) + } + }) + }) + } + t.Run("refused", func(t *testing.T) { + compattest.Both(t, func(t *testing.T) { + if !compat.V15() { + return + } + extensionFixture(t, "prod") + localPluginRegistry(t) + swapSeams(t, running) + err := reconcileAfterExtension(pluginInstallCmd, trigger) + if err == nil || errors.Is(err, appready.ErrCommitted) { + t.Fatalf("err = %v, want a pre-commit refusal", err) + } + }) + }) +} diff --git a/internal/bundle/appready/hooks.go b/internal/bundle/appready/hooks.go index 82bf706ef..8a7323fcd 100644 --- a/internal/bundle/appready/hooks.go +++ b/internal/bundle/appready/hooks.go @@ -26,6 +26,27 @@ import ( // plugins are installed, but there is no stack to reconcile, start or check. var ErrNoProject = errors.New("not in an nSelf project") +// ErrCommitted marks a Reconcile failure that happened after the generated +// files were written (a container swap that did not finish). The installed +// plugins are then referenced by committed configuration, so the install must +// not be undone. Wrap with Committed; test with errors.Is. +var ErrCommitted = errors.New("generated files were already written") + +type committedError struct{ err error } + +func (c *committedError) Error() string { return c.err.Error() } +func (c *committedError) Unwrap() error { return c.err } +func (c *committedError) Is(target error) bool { return target == ErrCommitted } + +// Committed marks err as a failure after the generated files were committed; +// the message and the wrapped chain are unchanged. A nil err stays nil. +func Committed(err error) error { + if err == nil { + return nil + } + return &committedError{err: err} +} + // Hooks are the project-level operations the post-install steps call. // A nil function skips its step and marks the report unverified. type Hooks struct { diff --git a/internal/bundle/installer_appready.go b/internal/bundle/installer_appready.go index a82ed1f38..ac8b4a7c0 100644 --- a/internal/bundle/installer_appready.go +++ b/internal/bundle/installer_appready.go @@ -128,7 +128,9 @@ func finishInstall(ctx context.Context, rn *appready.Runner, store *ledger.Store } if f := rn.Failure(); f != nil { verdict := appReadyVerdict(out, result.Bundle, f) - if f.Step == appready.StepReconcile { + // A failure after the generated files were written keeps the plugins: + // that configuration references them. + if f.Step == appready.StepReconcile && !errors.Is(f.Err, appready.ErrCommitted) { undoErr := undoFreshMembers(ctx, store, pluginDir, planned, prior, result, before, out) return errors.Join(verdict, undoErr) } diff --git a/internal/bundle/installer_appready_undo_test.go b/internal/bundle/installer_appready_undo_test.go index 907958a10..637ac95a5 100644 --- a/internal/bundle/installer_appready_undo_test.go +++ b/internal/bundle/installer_appready_undo_test.go @@ -172,3 +172,36 @@ func TestBundleInstallRefusedReconcileKeepsPreexistingMember(t *testing.T) { t.Error("notifications is still in the ledger") } } + +// A reconcile that fails after Apply wrote the generated files (a container +// swap error) must keep the plugins and their ledger records: the committed +// configuration references them. Only a pre-commit failure is undone. +func TestBundleInstallPostCommitReconcileFailureKeepsMembers(t *testing.T) { + pluginDir := isolate(t) + setupOfflineRegistry(t, mockTaskRegistry()) + var out bytes.Buffer + opts := freeOpts(t, pluginDir, &out, func(name string) error { + writeMember(t, pluginDir, name, 1, []string{"np_kept_data"}) + return nil + }) + cause := errors.New("orphaned containers were removed but the new services did not start") + opts.Hooks = refusedHooks(t) + opts.Hooks.Reconcile = func(context.Context) error { return appready.Committed(cause) } + + res, err := Install(context.Background(), "task", opts) + if err == nil || !errors.Is(err, cause) || !strings.Contains(err.Error(), "reconcile failed after install") { + t.Fatalf("err = %v, want the reconcile error", err) + } + for _, name := range []string{"notifications", "jobs"} { + if _, statErr := os.Stat(filepath.Join(pluginDir, name)); statErr != nil { + t.Errorf("member %q was removed after the files were committed: %v", name, statErr) + } + } + l, _, lerr := ledger.NewStore(pluginDir).Load() + if lerr != nil || len(l.Bundles) != 1 || len(l.Plugins) != 2 { + t.Errorf("ledger after a post-commit failure: err=%v bundles=%v plugins=%v", lerr, l.Bundles, l.Plugins) + } + if len(res.RolledBack) != 0 || !res.Changed { + t.Errorf("RolledBack=%v Changed=%v, want nothing rolled back and the change kept", res.RolledBack, res.Changed) + } +} From 461517a71a5f0ce7e9cb66e17ab8105fee17dbe6 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 15:20:21 -0400 Subject: [PATCH 5/7] fix(bundle): keep installed members when the state record write fails after publication (D-0760) --- cmd/commands/bundle_reconcile.go | 7 +++- cmd/commands/bundle_reconcile_orphans_test.go | 33 +++++++++++++++++ internal/reconcile/apply.go | 5 +++ internal/reconcile/published.go | 29 +++++++++++++++ internal/reconcile/state_test.go | 35 +++++++++++++++++++ 5 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 internal/reconcile/published.go diff --git a/cmd/commands/bundle_reconcile.go b/cmd/commands/bundle_reconcile.go index 668d356a1..0f89e5b32 100644 --- a/cmd/commands/bundle_reconcile.go +++ b/cmd/commands/bundle_reconcile.go @@ -67,7 +67,12 @@ func reconcileAfterExtension(cmd *cobra.Command, trigger reconcile.Trigger) erro return errs.New("E403", "extension files changed but regeneration needs confirmation"). WithWhy(err.Error()).WithFix("review the pending change with 'nself build --plan', then rerun with --yes (and --force for hand-edited files)") } - return fmt.Errorf("regenerating %s:%s: %w", trigger.Kind, trigger.Subject, err) + wrapped := fmt.Errorf("regenerating %s:%s: %w", trigger.Kind, trigger.Subject, err) + if errors.Is(err, reconcile.ErrPublished) { + // The files were written and kept: a bundle install keeps its plugins. + return appready.Committed(wrapped) + } + return wrapped } // Apply wrote the generated files: from here a failure is post-commit, and // a bundle install must keep its plugins (the files reference them). diff --git a/cmd/commands/bundle_reconcile_orphans_test.go b/cmd/commands/bundle_reconcile_orphans_test.go index 9a5c8dfc2..1b2608e3a 100644 --- a/cmd/commands/bundle_reconcile_orphans_test.go +++ b/cmd/commands/bundle_reconcile_orphans_test.go @@ -201,3 +201,36 @@ func TestExtensionReconcileSwapFailureIsCommitted(t *testing.T) { }) }) } + +// Apply publishes the generated files and then fails to write its state record: +// the files stay, so the error must carry the commit mark and a bundle install +// keeps its plugins (the review finding on D-0760). +func TestExtensionReconcileRecordFailureIsCommitted(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions") + } + trigger := reconcile.Trigger{Kind: reconcile.TriggerBundle, Subject: "zzfixture"} + compattest.Both(t, func(t *testing.T) { + if !compat.V15() { + return + } + p := extensionFixture(t, "") + localPluginRegistry(t) + swapSeams(t, fakeSwapRuntime{}) + stateDir := filepath.Dir(reconcile.GeneratedStatePath(p.project)) + if err := os.MkdirAll(stateDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(stateDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(stateDir, 0o755) }) + err := reconcileAfterExtension(pluginInstallCmd, trigger) + if err == nil || !errors.Is(err, appready.ErrCommitted) { + t.Fatalf("err = %v, want a post-commit failure", err) + } + if _, statErr := os.Stat(filepath.Join(p.project, "docker-compose.yml")); statErr != nil { + t.Fatalf("the published compose file is missing: %v", statErr) + } + }) +} diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index 474149f34..24e3802c0 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -203,6 +203,11 @@ func ApplyBuild(ctx context.Context, req Request, opt ApplyOptions) (applied *Pl // the record artifact, so plan == apply holds for the record too. firstRecorded := state == nil if err := writeRecord(req.ProjectDir, c.record); err != nil { + // The files stay unless the caller's Rollback (run by the deferred + // rollbackOnFailure) restores them; only then is the failure not post-publication. + if opt.Rollback == nil { + err = published(err) + } return nil, nil, err } if firstRecorded && req.Stderr != nil { diff --git a/internal/reconcile/published.go b/internal/reconcile/published.go new file mode 100644 index 000000000..5d9b5ce55 --- /dev/null +++ b/internal/reconcile/published.go @@ -0,0 +1,29 @@ +package reconcile + +// Purpose: tell a caller that an apply failed after the generated files were +// published. Inputs: the failure; outputs: an error that matches ErrPublished. +// Constraints: only a failure that leaves the published files on disk carries +// the mark; a refusal or a rolled-back write never does. + +import "errors" + +// ErrPublished marks an Apply failure that happened after the generated files +// were written and left in place (the state record could not be written). A +// caller that installed plugins for this apply must keep them: the files +// reference them. Test with errors.Is. +var ErrPublished = errors.New("generated files were already published") + +type publishedError struct{ err error } + +func (p *publishedError) Error() string { return p.err.Error() } +func (p *publishedError) Unwrap() error { return p.err } +func (p *publishedError) Is(target error) bool { return target == ErrPublished } + +// published marks err as a failure after publication; the message and the +// wrapped chain are unchanged. A nil err stays nil. +func published(err error) error { + if err == nil { + return nil + } + return &publishedError{err: err} +} diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index dee168843..c66050c7f 100644 --- a/internal/reconcile/state_test.go +++ b/internal/reconcile/state_test.go @@ -503,3 +503,38 @@ func TestStateApplyRecordWriteFails(t *testing.T) { t.Fatalf("the old record must stay in place (%v)", err) } } + +// A record that cannot be written fails the apply after the generated files are +// published and kept: the error must match ErrPublished so a caller that +// installed plugins for this apply keeps them. A failure before the write must not. +func TestApplyRecordWriteFailureIsPublished(t *testing.T) { + f := loadFixture(t, "dev-minimal") + _ = os.RemoveAll(filepath.Join(f.project, ".nself", "state")) + oldBuild := buildBuild + t.Cleanup(func() { buildBuild = oldBuild }) + buildBuild = func(projectDir string, opts nbuild.BuildOptions) (*nbuild.BuildResult, error) { + if err := os.WriteFile(filepath.Join(projectDir, "docker-compose.yml"), []byte("published"), 0o644); err != nil { + return nil, err + } + // The record path now holds a non-empty directory: the rename fails. + if err := os.MkdirAll(filepath.Join(GeneratedStatePath(projectDir), "occupied"), 0o755); err != nil { + return nil, err + } + return &nbuild.BuildResult{}, nil + } + req := Request{Runtime: &fakeRuntime{}, ProjectDir: f.project, Seed: []byte("seed")} + _, err := Apply(context.Background(), req, ApplyOptions{Yes: true}) + if err == nil || !errors.Is(err, ErrPublished) { + t.Fatalf("err = %v, want an error matching ErrPublished", err) + } + if _, statErr := os.Stat(filepath.Join(f.project, "docker-compose.yml")); statErr != nil { + t.Fatalf("the published file is gone: %v", statErr) + } + + // A write build that fails publishes nothing the caller must keep. + buildBuild = func(string, nbuild.BuildOptions) (*nbuild.BuildResult, error) { return nil, errors.New("write failed") } + _ = os.RemoveAll(filepath.Join(f.project, ".nself", "state")) + if _, err := Apply(context.Background(), req, ApplyOptions{Yes: true}); err == nil || errors.Is(err, ErrPublished) { + t.Fatalf("err = %v, want a failure without ErrPublished", err) + } +} From 9b8624af398215dacd061d626f5ade3684627a49 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 15:50:36 -0400 Subject: [PATCH 6/7] fix(reconcile): mark an E125 entitlement refresh failure as published so bundle install keeps its members (D-0760) --- internal/reconcile/apply.go | 8 +++++++ internal/reconcile/state_test.go | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index 24e3802c0..cbf3f2a9c 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -17,6 +17,7 @@ import ( "crypto/rand" "crypto/sha256" "encoding/binary" + "errors" "fmt" "io" @@ -193,6 +194,13 @@ func ApplyBuild(ctx context.Context, req Request, opt ApplyOptions) (applied *Pl wopts.Force = wopts.Force || !p.Empty res, err := buildWithEnv(req.ProjectDir, wopts, req.resolvedEnv, buildBuild) if err != nil { + // An entitlement refresh (E125) failure comes back after Build wrote and + // committed every generated file, so unless the caller's Rollback + // restores them the failure is post-publication. + var refresh *nbuild.EntitlementRefreshError + if res != nil && opt.Rollback == nil && errors.As(err, &refresh) { + err = published(err) + } return nil, nil, err } // P7-LIVE-04 (EPIC D5): the record is written only now that the whole diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index c66050c7f..43e2cb6b9 100644 --- a/internal/reconcile/state_test.go +++ b/internal/reconcile/state_test.go @@ -13,6 +13,7 @@ import ( "testing" nbuild "github.com/nself-org/cli/internal/build" + "github.com/nself-org/cli/internal/errs" ) func TestStateFaultInjection(t *testing.T) { @@ -538,3 +539,42 @@ func TestApplyRecordWriteFailureIsPublished(t *testing.T) { t.Fatalf("err = %v, want a failure without ErrPublished", err) } } + +// An E125 entitlement refresh failure comes back from Build after every +// generated file was written, with a completed result. The apply fails but the +// files stay, so the error must match ErrPublished; a failure with no result +// (nothing committed) or with a caller Rollback that restores the files must not. +func TestApplyEntitlementRefreshFailureIsPublished(t *testing.T) { + f := loadFixture(t, "dev-minimal") + _ = os.RemoveAll(filepath.Join(f.project, ".nself", "state")) + oldBuild := buildBuild + t.Cleanup(func() { buildBuild = oldBuild }) + refresh := &nbuild.EntitlementRefreshError{Err: errs.New("E125", "plugin entitlement unavailable")} + buildBuild = func(projectDir string, opts nbuild.BuildOptions) (*nbuild.BuildResult, error) { + if err := os.WriteFile(filepath.Join(projectDir, "docker-compose.yml"), []byte("published"), 0o644); err != nil { + return nil, err + } + return &nbuild.BuildResult{}, refresh + } + req := Request{Runtime: &fakeRuntime{}, ProjectDir: f.project, Seed: []byte("seed")} + _, err := Apply(context.Background(), req, ApplyOptions{Yes: true}) + if err == nil || !errors.Is(err, ErrPublished) { + t.Fatalf("err = %v, want an error matching ErrPublished", err) + } + if !errors.Is(err, refresh) { + t.Fatalf("err = %v, want the E125 cause kept on the chain", err) + } + + // The caller's Rollback restores the files: nothing stays published. + rolled := false + opt := ApplyOptions{Yes: true, Rollback: func() error { rolled = true; return nil }} + if _, err := Apply(context.Background(), req, opt); err == nil || errors.Is(err, ErrPublished) || !rolled { + t.Fatalf("err = %v, rolled = %v, want a rolled-back failure without ErrPublished", err, rolled) + } + + // No completed result: Build stopped before it committed. + buildBuild = func(string, nbuild.BuildOptions) (*nbuild.BuildResult, error) { return nil, refresh } + if _, err := Apply(context.Background(), req, ApplyOptions{Yes: true}); err == nil || errors.Is(err, ErrPublished) { + t.Fatalf("err = %v, want a failure without ErrPublished", err) + } +} From 6bcb65b3d01942e967c8dc202bffe699ea309b86 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Thu, 29 Oct 2026 08:00:08 -0400 Subject: [PATCH 7/7] fix(reconcile): preserve publication safeguards within code budgets --- internal/build/expect_sink.go | 21 +++-------- internal/build/proxy_restore.go | 61 ++++++++++++-------------------- internal/build/sink.go | 22 +++--------- internal/build/sink_remove.go | 12 +++++++ internal/reconcile/apply.go | 14 ++------ internal/reconcile/published.go | 16 ++++++++- internal/reconcile/state_test.go | 7 ++++ 7 files changed, 66 insertions(+), 87 deletions(-) create mode 100644 internal/build/sink_remove.go diff --git a/internal/build/expect_sink.go b/internal/build/expect_sink.go index 2531e039e..82b7b309b 100644 --- a/internal/build/expect_sink.go +++ b/internal/build/expect_sink.go @@ -1,19 +1,8 @@ package build -// expect_sink.go — a write-mode Sink that holds a build to a confirmed render -// (P7-LIVE-03, review M3 / EPIC ruling B). -// -// Purpose: `nself build` shows a plan, asks for confirmation, then writes. The -// write must be that render and nothing else, even if an input changed after -// the plan was made. expectSink stages the render: every write must carry -// exactly the bytes the confirmed render planned for that path, every removal -// and chmod must be one it planned, and anything else stops the build with an -// error before publishing any artifact. After the build, verify checks the -// complete staged render; commit publishes it with rollback on failure. -// Inputs: the PlannedBuild of the confirmed render (BuildOptions.Expect). -// Outputs: errors naming the path that deviated. -// Constraints: write mode only; reads see staged writes before disk. -// Effects are not asserted here (they run through writeEffects). +// expectSink stages exactly the confirmed render (P7-LIVE-03): unplanned bytes, +// removals or modes fail before publication. Reads overlay staged writes on disk. +// Commit verifies completeness and publishes with rollback; host effects are separate. import ( "bytes" @@ -28,13 +17,11 @@ import ( "github.com/nself-org/cli/internal/errs" ) -// deviationError is a write, removal or chmod that is not part of the -// confirmed render. +// deviationError identifies an operation outside the confirmed render. type deviationError struct{ msg string } func (e *deviationError) Error() string { return e.msg } -// errDeviated builds a deviationError. func errDeviated(path, why string) error { return &deviationError{msg: fmt.Sprintf("the project changed after the plan was made: %s %s; nothing further was written (re-run nself build --plan)", path, why)} } diff --git a/internal/build/proxy_restore.go b/internal/build/proxy_restore.go index 6d0918107..0a93ebb80 100644 --- a/internal/build/proxy_restore.go +++ b/internal/build/proxy_restore.go @@ -1,15 +1,7 @@ package build -// Purpose: return the generated tree to its pre-swap shape when the provider is -// nginx again (EPIC DEPL D4/D5): drop the renderer's two output directories, -// .nself/generated/traefik/ and .nself/generated/traefik-static/, if, and only -// if, the traefik renderer wrote them. -// Inputs: the build sink, the project workdir and the derived provider. -// Outputs: the directory's files removed through the sink (so plan mode only -// records them), or one warning when the directory is not provably ours. -// Constraints: never touches a path outside those two directories; a symlinked directory, a symlinked path component or a missing/foreign marker -// leaves everything in place; no Docker calls. Removal is per file, never an -// mv-replace of the directory (bind-mount gotcha 7). +// Restore nginx by removing only marked, generated traefik output through the +// sink. Foreign directories and symlinked paths stay; no Docker calls. import ( "bytes" @@ -23,26 +15,22 @@ import ( "sort" ) -// proxyGeneratedMarker is the file the traefik renderer writes beside its output; -// its text starts with proxyMarkerPrefix (plugins:free/traefik/cmd/render). +// The traefik renderer marks its output with this file and prefix. const ( proxyGeneratedMarker = ".nself-generated" proxyMarkerPrefix = "GENERATED BY" ) -// proxyOutputNames are the directories the traefik-config one-shot renders into -// (dynamic file-provider config, static config). Both hold the marker. +// Both dynamic and static traefik output directories hold a marker. var proxyOutputNames = []string{"traefik", "traefik-static"} -// proxyOutputDir is where the traefik plugin renders into. func proxyOutputDir(workdir string) string { return proxyOutputPath(workdir, "traefik") } func proxyOutputPath(workdir, name string) string { return filepath.Join(workdir, ".nself", "generated", name) } -// cleanupProviderOutput removes the output of a provider that no longer serves. -// It only acts for nginx; with traefik the plugin owns the directories. +// cleanupProviderOutput removes marked traefik output when switching to nginx. func cleanupProviderOutput(sink Sink, workdir, provider string) error { if provider != ProviderNginx { return nil @@ -72,16 +60,13 @@ func cleanupOutputDir(sink Sink, workdir, dir string) error { } return proxyKeep(dir, why) } - // Decide before any removal so plan and apply see the same thing: the - // plan-checked sink fails a build that planned a removal which did not happen. + // Plan and apply must agree on removals even for unwritable output. if why := unwritableIn(filepath.Dir(dir), dir); why != "" { return proxyKeep(dir, "it is owned by root (the traefik-config container wrote it): "+why+" is not writable; run `sudo rm -rf "+dir+"` or chown it to your user, then build again") } err = removeTree(sink, dir, info) if errors.Is(err, fs.ErrPermission) { - // The one-shot runs as root and Docker creates its bind directories - // root-owned, so an unprivileged build cannot unlink inside them. The - // marker is removed last, so the next build retries. + // Root-owned container output may need manual cleanup; leave its marker. return proxyKeep(dir, "it is owned by root (the traefik-config container wrote it): "+err.Error()+"; run `sudo rm -rf "+dir+"` or chown it to your user, then build again") } if err != nil { @@ -90,8 +75,7 @@ func cleanupOutputDir(sink Sink, workdir, dir string) error { return nil } -// unwritableIn returns the first directory, the parent first and then dir and -// everything under it, in which this user cannot unlink, or "". +// unwritableIn finds the first directory that prevents unlinking, parent first. func unwritableIn(parent, dir string) string { if !dirWritable(parent) { return parent @@ -116,16 +100,14 @@ func unwritableIn(parent, dir string) string { return bad } -// proxyKeep warns once that dir stays and why. The build goes on: a leftover -// the renderer did not write is not this build's to delete. +// proxyKeep warns and continues without deleting unproven output. func proxyKeep(dir, why string) error { slog.Warn("leaving "+dir+" in place: "+why, "fix", "remove it by hand if it is stale; nself deletes it only when the traefik renderer's "+proxyGeneratedMarker+" marker is inside") return nil } -// proxyOwnedDir reports whether dir is exactly /.nself/generated/traefik -// with no symlink on the way and a regular renderer marker inside. +// proxyOwnedDir requires an unsymlinked output path and a regular renderer marker. func proxyOwnedDir(workdir, dir string) (bool, string) { root, err := filepath.EvalSymlinks(workdir) if err != nil { @@ -152,9 +134,9 @@ func proxyOwnedDir(workdir, dir string) (bool, string) { return true, "" } -// removeTree deletes dir's files and then dir, through the sink. A symlink -// entry is removed as a link, never followed. The marker goes last, so an -// interrupted cleanup still leaves a directory the next build recognises. +var errTreeSwapped = errors.New("a directory was replaced while it was being removed") + +// removeTree confines unlinks to the checked directory; markers go last for retry. func removeTree(sink Sink, dir string, before fs.FileInfo) error { if !before.IsDir() { return fmt.Errorf("cleanup directory %s was replaced", dir) @@ -177,18 +159,21 @@ func removeTree(sink Sink, dir string, before fs.FileInfo) error { return sink.Remove(dir) } -// removeTreeIn anchors both traversal and disk unlinks to the generated output -// directory. A swapped symlink cannot redirect either operation to a sibling. +// removeTreeIn checks each listing against the anchored directory identity. +// Disk unlinks stay confined to the open generated output directory. func removeTreeIn(sink Sink, root *os.Root, dir, relative string) error { - f, err := root.Open(relative) + before, err := root.Lstat(relative) if err != nil { return err } - entries, err := f.ReadDir(-1) - _ = f.Close() + entries, err := sink.ReadDir(filepath.Join(dir, relative)) if err != nil { return err } + after, err := os.Lstat(filepath.Join(dir, relative)) + if err != nil || !after.IsDir() || !os.SameFile(before, after) { + return errTreeSwapped + } sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) for _, e := range entries { rel := filepath.Join(relative, e.Name()) @@ -223,9 +208,7 @@ func removeTreeIn(sink Sink, root *os.Root, dir, relative string) error { return nil } -// emptyPrecreatedDir reports whether dir is exactly /.nself/generated/, -// reached through no symlink, and holds nothing: the leftover of a build that -// pre-created it for a renderer that never ran. Removing it deletes no data. +// emptyPrecreatedDir accepts only empty, unsymlinked renderer output paths. func emptyPrecreatedDir(workdir, dir string) bool { root, err := filepath.EvalSymlinks(workdir) if err != nil { diff --git a/internal/build/sink.go b/internal/build/sink.go index 8b3aecefd..ca3504ff4 100644 --- a/internal/build/sink.go +++ b/internal/build/sink.go @@ -1,14 +1,8 @@ package build -// sink.go — Sink serves write and plan modes (P7-LIVE-21). -// -// Purpose: diskSink writes; memSink records mutations and overlays disk reads. -// Inputs: project-relative or absolute paths, canonicalised relative to the -// project root or as "@fronting/" for the fronting nginx/sites directory. -// Outputs: PlannedBuild (files, modes, removals, effects) from memSink. -// Constraints: memSink never writes or creates a directory; internal/build must -// not import internal/reconcile (P7-GUARD), so the store is a plain PlannedFile -// map that P7-LIVE-03 converts. +// Sink serves write and plan modes (P7-LIVE-21). diskSink writes; memSink +// overlays mutations without disk writes. Keys are project-relative, absolute, +// or @fronting paths. Plain PlannedBuild data avoids importing reconcile. import ( "errors" @@ -71,10 +65,7 @@ func (d *diskSink) WriteFile(p string, data []byte, perm fs.FileMode) error { func (d *diskSink) WriteAtomic(p string, data []byte, perm fs.FileMode) error { return atomicWrite(d.abs(p), data, perm) } -func (d *diskSink) Remove(p string) error { return os.Remove(d.abs(p)) } -func (d *diskSink) RemoveWithin(root *os.Root, relative, _ string) error { - return root.Remove(relative) -} +func (d *diskSink) Remove(p string) error { return os.Remove(d.abs(p)) } func (d *diskSink) Chmod(p string, perm fs.FileMode) error { return os.Chmod(d.abs(p), perm) } func (d *diskSink) MkdirAll(p string, perm fs.FileMode) error { return os.MkdirAll(d.abs(p), perm) } func (d *diskSink) ReadFile(p string) ([]byte, error) { return os.ReadFile(d.abs(p)) } @@ -178,10 +169,6 @@ func (m *memSink) Remove(p string) error { return nil } -func (m *memSink) RemoveWithin(_ *os.Root, _ string, p string) error { - return m.Remove(p) -} - func (m *memSink) Chmod(p string, perm fs.FileMode) error { k := m.key(p) if f, ok := m.files[k]; ok { @@ -293,7 +280,6 @@ func (m *memSink) snapshot() *PlannedBuild { return pb } -// memInfo is the fs.FileInfo of a planned file or directory. type memInfo struct { name string size int64 diff --git a/internal/build/sink_remove.go b/internal/build/sink_remove.go new file mode 100644 index 000000000..b62f8ee16 --- /dev/null +++ b/internal/build/sink_remove.go @@ -0,0 +1,12 @@ +package build + +// Cleanup removals use an open root so swapped paths cannot escape it. +import "os" + +func (d *diskSink) RemoveWithin(root *os.Root, relative, _ string) error { + return root.Remove(relative) +} + +func (m *memSink) RemoveWithin(_ *os.Root, _ string, p string) error { + return m.Remove(p) +} diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index cbf3f2a9c..afc5cb044 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -17,7 +17,6 @@ import ( "crypto/rand" "crypto/sha256" "encoding/binary" - "errors" "fmt" "io" @@ -194,13 +193,7 @@ func ApplyBuild(ctx context.Context, req Request, opt ApplyOptions) (applied *Pl wopts.Force = wopts.Force || !p.Empty res, err := buildWithEnv(req.ProjectDir, wopts, req.resolvedEnv, buildBuild) if err != nil { - // An entitlement refresh (E125) failure comes back after Build wrote and - // committed every generated file, so unless the caller's Rollback - // restores them the failure is post-publication. - var refresh *nbuild.EntitlementRefreshError - if res != nil && opt.Rollback == nil && errors.As(err, &refresh) { - err = published(err) - } + err = buildFailure(err, res, opt) return nil, nil, err } // P7-LIVE-04 (EPIC D5): the record is written only now that the whole @@ -278,16 +271,13 @@ func short(id string) string { return id } -// seededRand is a SHA-256 counter-mode byte stream: the same seed yields the -// same bytes, and the stream is as strong as the seed (32 random bytes from -// crypto/rand in Apply). +// seededRand is a SHA-256 counter-mode byte stream seeded by crypto/rand. type seededRand struct { seed []byte ctr uint64 buf []byte } -// newSeededRand returns a reader over the stream of seed. func newSeededRand(seed []byte) io.Reader { return &seededRand{seed: append([]byte(nil), seed...)} } diff --git a/internal/reconcile/published.go b/internal/reconcile/published.go index 5d9b5ce55..9fdc00aae 100644 --- a/internal/reconcile/published.go +++ b/internal/reconcile/published.go @@ -5,7 +5,11 @@ package reconcile // Constraints: only a failure that leaves the published files on disk carries // the mark; a refusal or a rolled-back write never does. -import "errors" +import ( + "errors" + + nbuild "github.com/nself-org/cli/internal/build" +) // ErrPublished marks an Apply failure that happened after the generated files // were written and left in place (the state record could not be written). A @@ -27,3 +31,13 @@ func published(err error) error { } return &publishedError{err: err} } + +// buildFailure preserves publication status when entitlement refresh failed +// after Build committed its artifacts. A caller rollback restores the files. +func buildFailure(err error, res *nbuild.BuildResult, opt ApplyOptions) error { + var refresh *nbuild.EntitlementRefreshError + if res != nil && opt.Rollback == nil && errors.As(err, &refresh) { + return published(err) + } + return err +} diff --git a/internal/reconcile/state_test.go b/internal/reconcile/state_test.go index 43e2cb6b9..e7c9e3c9f 100644 --- a/internal/reconcile/state_test.go +++ b/internal/reconcile/state_test.go @@ -565,6 +565,13 @@ func TestApplyEntitlementRefreshFailureIsPublished(t *testing.T) { t.Fatalf("err = %v, want the E125 cause kept on the chain", err) } + if data, readErr := os.ReadFile(filepath.Join(f.project, "docker-compose.yml")); readErr != nil || string(data) != "published" { + t.Fatalf("published artifacts must remain after E125: %q, %v", data, readErr) + } + if detail := errs.Describe(err); detail == nil || detail.Code != "E125" || errs.ExitCodeFor(err) != 3 { + t.Fatalf("publication mark lost the entitlement error identity: %v", err) + } + // The caller's Rollback restores the files: nothing stays published. rolled := false opt := ApplyOptions{Yes: true, Rollback: func() error { rolled = true; return nil }}