v0.285.0 — a box keeps two controller versions (decision 56, R-745); the update clean-up's nil-stack crash (R-751)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS

controller/internal/stacks/controller_image_retention.go

@@ -0,0 +1,188 @@
+package stacks
+
+import (
+	"fmt"
+	"sort"
+	"strings"
+
+	"gitea.dooplex.hu/admin/felhom-controller/internal/util"
+)
+
+// ── Controller image retention (R-745, `09` §3 decision 56) ───────────────────────────────────────────
+//
+// Decision 53's app sweep never touches the controller's own repository (deleteUnkeptImages skips it), so every
+// release a box ever ran stayed: ~80 controller images on demo-hp's guest on 2026-10-01. The ruling: a box keeps the
+// controller image it RUNS and the one BEFORE it; older ones are deleted by the same in-use rule as decision 53.
+//
+// MEASURED before building (2026-10-01, `audits/rulings-2026-10-01/B/`):
+//   - the agent's swap rolls back to whatever /etc/felhom-controller-image named when the swap began — the RUNNING
+//     image (felhom-agent internal/localapi/controllerswap.go Swap/rollback). The controller does NOT hand it a
+//     previous image; SwapController carries the target only. So the self-update's own roll-back needs only the
+//     running image, which a container uses and is never a candidate. "The previous" is kept for a hand roll-back.
+//   - the bootstrap unit `docker run`s the image the file names at every guest boot; the file always names the
+//     running image after a swap (done or rolled back). The golden's baked image is the running one until the first
+//     swap, and nothing reads it after that.
+//   - a whole-guest restore brings /var/lib/docker back with the guest (mp0 backup=1), so the image it ran then.
+//
+// THE KEEP SET (rebuilt at every pass): every image a container uses (the running controller among them); the tag
+// of the running version; the PREVIOUS — the self-update's own record (update-state.json previous_image of a swap
+// that ended on the running version), or, when there is no such record or its image is gone, the highest version
+// below the running one (logged as "by version order"); every version ABOVE the running one (a pulled target the
+// swap has not taken yet); every tag that is not a plain X.Y.Z (latest, -rc) — left alone, logged. A candidate is an
+// untagged (<none>) controller image or one whose every tag is a version below the previous. Deleted by exact
+// name/ID, never forced, never by prune; an ID that also carries another repository's name is left alone. NOTHING is
+// deleted while the controller is swapping itself (selfUpdatingNow). Registry tags are never touched.
+// Pinned by internal/stacks/controller_image_retention_test.go.
+
+// ControllerImageRecord is what the caller knows about the controller's own images.
+type ControllerImageRecord struct {
+	Repo     string // the controller repository, no tag (cfg.SelfUpdate.Image)
+	Running  string // this process's version
+	Previous string // the self-update's record: the image before Running ("" when none)
+}
+
+// RetainControllerImages applies decision 56 once. Returns the names it deleted.
+func (m *Manager) RetainControllerImages(rec ControllerImageRecord) ([]string, error) {
+	imageRetentionMu.Lock()
+	defer imageRetentionMu.Unlock()
+	running, err := util.ParseVersion(rec.Running)
+	if err != nil || rec.Repo == "" {
+		m.logger.Printf("[INFO] [stacks] controller image retention: skipped — running version %q is not a release (or no repository)", rec.Running)
+		return nil, nil
+	}
+	if m.selfUpdatingNow() {
+		m.logger.Printf("[INFO] [stacks] controller image retention: skipped — the controller is swapping itself (the target it pulled is named by nothing yet)")
+		return nil, nil
+	}
+	imgs, err := listLocalImages()
+	if err != nil {
+		return nil, err
+	}
+	used, err := imagesUsedByContainers()
+	if err != nil {
+		m.logger.Printf("[WARN] [stacks] controller image retention: the containers could not be read (%v) — NOTHING is deleted", err)
+		return nil, err
+	}
+	repo := normRepo(rec.Repo)
+	byID := map[string][]localImage{}
+	for _, im := range imgs {
+		byID[im.ID] = append(byID[im.ID], im)
+	}
+
+	// The previous: the swap's own record first, version order only when the record names nothing present.
+	prevTag, prevHow := "", ""
+	if rec.Previous != "" {
+		if r, t, _ := splitRepoTag(rec.Previous); normRepo(r) == repo {
+			for _, im := range imgs {
+				if im.Repo == repo && im.Tag == t {
+					prevTag, prevHow = t, "the self-update's record"
+					break
+				}
+			}
+		}
+	}
+	if prevTag == "" {
+		var best *util.Version
+		for _, im := range imgs {
+			if im.Repo != repo {
+				continue
+			}
+			if v, err := util.ParseVersion(im.Tag); err == nil && v.Compare(running) < 0 && (best == nil || v.Compare(*best) > 0) {
+				vv := v
+				best = &vv
+			}
+		}
+		if best != nil {
+			prevTag, prevHow = best.Raw, "by version order (no swap record names one present)"
+		}
+	}
+	var prev *util.Version
+	if prevTag != "" {
+		if v, err := util.ParseVersion(prevTag); err == nil {
+			prev = &v
+		}
+	}
+
+	ids := make([]string, 0, len(byID))
+	for id := range byID {
+		ids = append(ids, id)
+	}
+	sort.Strings(ids)
+	var deleted []string
+	considered, candidates := 0, 0
+	defer func() {
+		// ONE line per pass, whatever it did — the positive observable that the pass ran (R-96 rule 3).
+		m.logger.Printf("[INFO] [stacks] controller image retention: pass over %d controller image(s) — running %s, previous %q (%s), %d candidate(s), %d deleted, the rest kept",
+			considered, rec.Running, prevTag, prevHow, candidates, len(deleted))
+	}()
+	for _, id := range ids {
+		group := byID[id]
+		var mine []localImage
+		other := false
+		for _, im := range group {
+			if im.Repo == repo {
+				mine = append(mine, im)
+			} else {
+				other = true
+			}
+		}
+		if len(mine) == 0 {
+			continue
+		}
+		considered++
+		if used[id] {
+			continue
+		}
+		deletable, odd := true, ""
+		var names []string
+		for _, im := range mine {
+			if im.Tag == "<none>" || im.Tag == "" {
+				continue
+			}
+			names = append(names, im.Repo+":"+im.Tag)
+			v, err := util.ParseVersion(im.Tag)
+			switch {
+			case err != nil:
+				deletable, odd = false, im.Tag // latest, -rc: not ours to judge
+			case v.Compare(running) >= 0:
+				deletable = false // the running one, or a pulled target not swapped to yet
+			case prev == nil || v.Compare(*prev) >= 0:
+				deletable = false // the previous (or nothing to call previous: keep)
+			}
+		}
+		if !deletable {
+			if odd != "" {
+				m.logger.Printf("[INFO] [stacks] controller image retention: %s carries a tag that is not a version (%s) — left alone", shortID(id), odd)
+			}
+			continue
+		}
+		candidates++
+		if other {
+			m.logger.Printf("[INFO] [stacks] controller image retention: %s also carries another repository's name — left alone", shortID(id))
+			continue
+		}
+		// A tagged image is removed by its names (rmi by ID refuses an ID with several tags); an untagged one by ID.
+		targets := names
+		if len(targets) == 0 {
+			targets = []string{id}
+		}
+		ok := true
+		for _, t := range targets {
+			if out, err := imageDocker("rmi", t); err != nil {
+				m.logger.Printf("[WARN] [stacks] controller image retention: docker refused to delete %s (%s): %s", t, shortID(id), truncateStr(strings.TrimSpace(out), 160))
+				ok = false
+				break
+			}
+		}
+		if !ok {
+			continue
+		}
+		label := strings.Join(names, ",")
+		if label == "" {
+			label = repo + ":<none>"
+		}
+		m.logger.Printf("[INFO] [stacks] controller image retention: deleted %s (%s, %s) — older than the previous controller and no container uses it (decision 56)", label, shortID(id), mine[0].Size)
+		deleted = append(deleted, fmt.Sprintf("%s@%s", label, shortID(id)))
+	}
+	return deleted, nil
+}

controller/internal/stacks/controller_image_retention_test.go

@@ -0,0 +1,185 @@
+package stacks
+
+import (
+	"fmt"
+	"os"
+	"strings"
+	"testing"
+)
+
+// R-745 (decision 56): a box keeps the controller image it runs and the one before it; older controller images go,
+// by exact name/ID, after the in-use check; nothing goes while the controller swaps itself. Docker is the imageDocker
+// seam (fakeImages, image_retention_test.go) — nothing here reaches a daemon.
+
+const ctlRepo = "gitea.dooplex.hu/admin/felhom-controller"
+
+func ctlImages() *fakeImages {
+	return &fakeImages{
+		imgs: []localImage{
+			{ID: "sha256:C285", Repo: ctlRepo, Tag: "0.285.0", Size: "409MB"},
+			{ID: "sha256:C2842", Repo: ctlRepo, Tag: "0.284.2", Size: "409MB"},
+			{ID: "sha256:C2841", Repo: ctlRepo, Tag: "0.284.1", Size: "409MB"},
+			{ID: "sha256:C2831", Repo: ctlRepo, Tag: "0.283.1", Size: "409MB"},
+			{ID: "sha256:CNONE", Repo: ctlRepo, Tag: "<none>", Size: "422MB"},
+			{ID: "sha256:CRC", Repo: ctlRepo, Tag: "0.273.0-rc1", Size: "400MB"},
+			{ID: "sha256:W3", Repo: "acme/web", Tag: "3", Size: "100MB"},
+		},
+		containers: map[string]string{"c-ctl": "sha256:C285"},
+	}
+}
+
+func rmiSet(f *fakeImages) string { return "," + strings.Join(f.rmi, ",") + "," }
+
+// The running one and the one the swap record names stay; an older one and an untagged one go; another repository's
+// image and a non-version tag are never touched.
+// COMPANION RED-PROOF: drop the `prev` case from the keep switch → "the previous controller (0.284.2) was deleted".
+func TestControllerRetention_KeepsRunningAndPreviousDeletesOlder(t *testing.T) {
+	m := retentionManager(t)
+	f := ctlImages()
+	withFakeImages(t, f)
+	got, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.285.0", Previous: ctlRepo + ":0.284.2"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	s := rmiSet(f)
+	if strings.Contains(s, ":0.285.0,") || strings.Contains(s, "sha256:C285,") {
+		t.Fatalf("the RUNNING controller was deleted: %v", f.rmi)
+	}
+	if strings.Contains(s, ":0.284.2,") {
+		t.Fatalf("the previous controller (0.284.2) was deleted: %v", f.rmi)
+	}
+	for _, want := range []string{ctlRepo + ":0.284.1", ctlRepo + ":0.283.1", "sha256:CNONE"} {
+		if !strings.Contains(s, ","+want+",") {
+			t.Fatalf("%s (older than the previous / untagged) was not deleted: rmi=%v", want, f.rmi)
+		}
+	}
+	if strings.Contains(s, "0.273.0-rc1") || strings.Contains(s, "acme/web") || strings.Contains(s, "W3") {
+		t.Fatalf("a non-version tag or another repository's image was touched: %v", f.rmi)
+	}
+	if len(got) != 3 {
+		t.Fatalf("deleted %d, want 3: %v", len(got), got)
+	}
+}
+
+// The swap record wins over version order: a box rolled back by hand runs 0.285.0 with 0.283.1 as its recorded
+// previous — 0.284.x (newer by sort order) go, the recorded one stays.
+// COMPANION RED-PROOF: ignore rec.Previous (version order only) → "the recorded previous (0.283.1) was deleted".
+func TestControllerRetention_RecordBeatsVersionOrder(t *testing.T) {
+	m := retentionManager(t)
+	f := ctlImages()
+	withFakeImages(t, f)
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.285.0", Previous: ctlRepo + ":0.283.1"}); err != nil {
+		t.Fatal(err)
+	}
+	s := rmiSet(f)
+	if strings.Contains(s, ":0.283.1,") {
+		t.Fatalf("the recorded previous (0.283.1) was deleted: %v", f.rmi)
+	}
+	// 0.284.x are NEWER than the recorded previous: kept (only versions below the previous are candidates).
+	if strings.Contains(s, ":0.284.2,") || strings.Contains(s, ":0.284.1,") {
+		t.Fatalf("a version between the previous and the running one was deleted: %v", f.rmi)
+	}
+}
+
+// No record (a box set up by hand, like 9202): the highest version below the running one is the previous.
+func TestControllerRetention_NoRecordFallsBackToVersionOrder(t *testing.T) {
+	m := retentionManager(t)
+	f := ctlImages()
+	withFakeImages(t, f)
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.285.0"}); err != nil {
+		t.Fatal(err)
+	}
+	s := rmiSet(f)
+	if strings.Contains(s, ":0.284.2,") {
+		t.Fatalf("with no record, the highest older version (0.284.2) must stay: %v", f.rmi)
+	}
+	if !strings.Contains(s, ":0.284.1,") {
+		t.Fatalf("0.284.1 should go: %v", f.rmi)
+	}
+}
+
+// A version ABOVE the running one (a target the self-update pulled, not swapped to yet) is never deleted, and while
+// the controller swaps itself NOTHING is deleted.
+// COMPANION RED-PROOF: remove the selfUpdatingNow() check → "deleted while the controller is swapping itself".
+func TestControllerRetention_NothingWhileSwappingAndNewerKept(t *testing.T) {
+	m := retentionManager(t)
+	f := ctlImages()
+	f.containers = map[string]string{"c-ctl": "sha256:C2842"} // running 0.284.2, 0.285.0 pulled
+	withFakeImages(t, f)
+	m.SetSelfUpdatingCheck(func() bool { return true })
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.284.2", Previous: ctlRepo + ":0.284.1"}); err != nil {
+		t.Fatal(err)
+	}
+	if len(f.rmi) != 0 {
+		t.Fatalf("deleted while the controller is swapping itself: %v", f.rmi)
+	}
+	m.SetSelfUpdatingCheck(func() bool { return false })
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.284.2", Previous: ctlRepo + ":0.284.1"}); err != nil {
+		t.Fatal(err)
+	}
+	s := rmiSet(f)
+	if strings.Contains(s, ":0.285.0,") {
+		t.Fatalf("the pulled target (0.285.0, above the running one) was deleted: %v", f.rmi)
+	}
+	if strings.Contains(s, ":0.284.1,") || !strings.Contains(s, ":0.283.1,") {
+		t.Fatalf("want 0.284.1 kept (previous) and 0.283.1 deleted: %v", f.rmi)
+	}
+}
+
+// A container that still uses an OLD controller image keeps it (the in-use rule), and a container list that cannot
+// be read deletes nothing (fail closed).
+func TestControllerRetention_InUseAndFailClosed(t *testing.T) {
+	m := retentionManager(t)
+	f := ctlImages()
+	f.containers["c-old"] = "sha256:C2831"
+	withFakeImages(t, f)
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.285.0", Previous: ctlRepo + ":0.284.2"}); err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(rmiSet(f), ":0.283.1,") {
+		t.Fatalf("an image a container uses was deleted: %v", f.rmi)
+	}
+
+	f2 := ctlImages()
+	withFakeImages(t, &fakeImages{imgs: f2.imgs, containers: f2.containers})
+	prev := imageDocker
+	imageDocker = func(args ...string) (string, error) {
+		if args[0] == "inspect" {
+			return "", fmt.Errorf("no such container")
+		}
+		return prev(args...)
+	}
+	t.Cleanup(func() { imageDocker = prev })
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "0.285.0"}); err == nil {
+		t.Fatal("an unreadable container list must be an error (and delete nothing)")
+	}
+}
+
+// A dev build (no release version) does nothing.
+func TestControllerRetention_DevBuildSkips(t *testing.T) {
+	m := retentionManager(t)
+	f := ctlImages()
+	withFakeImages(t, f)
+	if _, err := m.RetainControllerImages(ControllerImageRecord{Repo: ctlRepo, Running: "dev"}); err != nil {
+		t.Fatal(err)
+	}
+	if len(f.rmi) != 0 {
+		t.Fatalf("a dev build deleted images: %v", f.rmi)
+	}
+}
+
+// R-751: the retention after an update runs in a goroutine; when the app is gone by the time it re-reads the stack
+// (removed right after the update, or a rescan that no longer finds it), it must return — it dereferenced a nil stack
+// and the panic took the whole controller down (seen 2026-10-01 in the full suite: a test's temp dir removed under it).
+// COMPANION RED-PROOF: without the `!ok` check after the rescan → panic "invalid memory address".
+func TestRetainImagesAfterUpdate_AppGoneDoesNotPanic(t *testing.T) {
+	m := retentionManager(t)
+	f := baseImages()
+	withFakeImages(t, f)
+	st, _ := m.GetStack("web")
+	must(t, os.Remove(st.ComposePath)) // the rescan inside RetainImagesAfterUpdate no longer finds "web"
+	m.RetainImagesAfterUpdate("web", map[string]InstalledImage{"web": {Ref: "acme/web:2", Digest: "sha256:w2"}})
+	if len(f.rmi) != 0 {
+		t.Fatalf("deleted images for an app that is gone: %v", f.rmi)
+	}
+}

controller/internal/stacks/image_retention.go

@@ -287,13 +287,18 @@ func (m *Manager) RetainImagesAfterUpdate(name string, previous map[string]Insta
 			m.logger.Printf("[WARN] [stacks] image retention %s: rescan failed: %v", name, err)
 		}
 	}
-	st, _ = m.GetStack(name)
+	// R-751: the app can be gone by now (removed right after the update, or the rescan no longer finds it) — a nil
+	// stack here panicked in this goroutine and took the controller down. Its images are then the remove's to judge.
+	if st, ok = m.GetStack(name); !ok || st == nil {
+		m.logger.Printf("[INFO] [stacks] image retention after the update of %s: the app is gone — nothing to do here", name)
+		return
+	}
 	if _, err := m.deleteUnkeptImages("update of "+name, appImageRepos(dir, st.AppConfig), ""); err != nil {
 		m.logger.Printf("[WARN] [stacks] image retention after the update of %s: %v", name, err)
 	}
 }
 
-// retainAfterUpdateFn runs the retention after an update ends (a seam: the update tests do not exercise it).
+// retainAfterUpdateFn runs the retention after an update ends (a seam: a no-op in this package's tests — TestMain, R-751).
 var retainAfterUpdateFn = func(m *Manager, name string, previous map[string]InstalledImage) {
 	go m.RetainImagesAfterUpdate(name, previous)
 }

controller/internal/stacks/r650_main_test.go

@@ -9,4 +9,13 @@ import (
 
 // TestMain puts a silent docker stub on PATH for every test in this package: R-650, the fixtures
 // here build a real stacks.Manager, and on the build host (DooPlex) that reached production Docker.
-func TestMain(m *testing.M) { os.Exit(dockerexec.RunWithStub(m)) }
+//
+// R-751: the retention after an update or a remove runs in a GOROUTINE that outlives the test that triggered it — it
+// wrote into a test's temp dir while the dir was being removed ("directory not empty") and once panicked on the
+// vanished stack. No test here may leave one running: both seams are no-ops by default; the retention tests call
+// RetainImagesAfter* directly, and the wiring tests swap the seams themselves.
+func TestMain(m *testing.M) {
+	retainAfterUpdateFn = func(*Manager, string, map[string]InstalledImage) {}
+	retainAfterRemoveFn = func(*Manager, string, map[string]bool) {}
+	os.Exit(dockerexec.RunWithStub(m))
+}