fix(cli): harden restore cleanup and bundle rollback (D-0286 D-1162 D-0760)

.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 <bundle> --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.

.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
 ```

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"
@@ -66,15 +67,22 @@ 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).
 	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.

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,87 @@ 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)
+			}
+		})
+	})
+}
+
+// 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)
+		}
+	})
+}

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 {

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
+}

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 {

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 <workdir>/.nself/generated/<name>,