From 861f8d3538aa47d8397a93f3d1a988405666eb16 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 10:59:42 -0400 Subject: [PATCH 1/6] 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 e7319c91a..c323671c3 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 adacf9de3..9fb54b7dd 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.Update(func(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.) @@ -114,7 +118,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 a4e70429709b8cd60140ee4b9fe8d17b29375fb9 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 11:04:42 -0400 Subject: [PATCH 2/6] 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 | 61 ++++++-- internal/build/proxy_restore_checked_test.go | 142 +++++++++++++++++++ internal/build/shared_proxy_sink.go | 5 + internal/build/sink.go | 15 +- 8 files changed, 287 insertions(+), 18 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 4cbcd2a81..dc0e5703e 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 0dcf5798a..01c03ec73 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; @@ -155,31 +156,75 @@ func proxyOwnedDir(workdir, dir string) (bool, string) { // entry is removed as a link, never followed. The marker goes last, so an // interrupted cleanup still leaves a directory the next build recognises. func removeTree(sink Sink, dir string) error { - entries, err := sink.ReadDir(dir) + before, err := os.Lstat(dir) if err != nil { return err } + 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 } // 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); 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 err := sink.Remove(filepath.Join(dir, proxyGeneratedMarker)); err != nil && !errors.Is(err, fs.ErrNotExist) { + marker := filepath.Join(relative, proxyGeneratedMarker) + if _, err := root.Lstat(marker); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } return err } - return sink.Remove(dir) + if err := sink.RemoveWithin(root, marker, filepath.Join(dir, marker)); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + 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 5109aba055ed9d698023f84e018d72af40991671 Mon Sep 17 00:00:00 2001 From: admin Date: Sun, 25 Oct 2026 11:27:57 -0400 Subject: [PATCH 3/6] 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 b11e99e0268d602352a1f7711b50eafdb50ce8d8 Mon Sep 17 00:00:00 2001 From: admin Date: Sun, 25 Oct 2026 15:00:56 -0400 Subject: [PATCH 4/6] 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 9fb54b7dd..f99d63ad7 100644 --- a/internal/bundle/installer_appready.go +++ b/internal/bundle/installer_appready.go @@ -119,7 +119,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 30f207c849b758795a361c078e3d5fe9ce92311b Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 15:20:21 -0400 Subject: [PATCH 5/6] 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 e96c91b5ff27f0fe66c36b5dcc25d223e3fefd39 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 15:50:36 -0400 Subject: [PATCH 6/6] 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) + } +}