diff --git a/CHANGELOG.md b/CHANGELOG.md index a2eb6d2..be81a0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,25 @@ +## v0.285.0 — a box keeps two controller versions (decision 56); a crash in the update clean-up fixed (2026-10-01) + +**MinAgent: 0.131.0** (unchanged). Needs hub v0.123.0 (unchanged). No new strings. + +- **Controller image retention — `09` §3 decision 56 (R-745).** Decision 53's sweep never touched the controller's + own repository, so every release a box ran stayed (demo-hp's guest: 83 controller images on 2026-10-01). Now, 3 + minutes after every start (`stacks/controller_image_retention.go`), the box keeps the controller image it runs, the + one before it — by the swap's own record (`update-state.json` `previous_image` of a swap that ended on the running + version, `selfupdate.UpdateState.RecordedPrevious`), by version order only when no record names one present — + every version ABOVE the running one (a pulled target not swapped to yet) and every non-version tag (`latest`, `-rc`). + Older and untagged controller images are deleted by exact name/ID after the in-use check; never forced, never pruned; + nothing while the controller swaps itself; registry tags never. One INFO line per pass, one per deletion with size. + **Measured first:** the agent's swap rolls back to what `/etc/felhom-controller-image` named when the swap began — the + RUNNING image — and is never handed a previous image; so the self-update's own roll-back needs only the running one. +- **R-751 (found by the full suite):** the retention after an update runs in a goroutine and re-reads the stack after a + rescan; when the app was gone by then it dereferenced a nil stack — a panic in a goroutine ends the controller. Now + it returns. The two retention seams are no-ops in the package's tests (`TestMain`), where the goroutine outlived its + test and wrote into a removed temp dir. +- Tests `TestControllerRetention_*` (6), `TestRecordedPrevious`, `TestRetainImagesAfterUpdate_AppGoneDoesNotPanic`; + red-proofs: drop the previous from the keep switch, ignore the record, drop the swap check, drop the in-use check, + drop the success check, drop the nil-stack check — each fails (`felhom.eu/documentation/audits/rulings-2026-10-01/B/`). + ## v0.284.2 — the image clean-up sees digest-pulled (untagged) images (2026-09-30) **MinAgent: 0.131.0** (unchanged). Needs hub v0.123.0 (unchanged). No new strings. v0.284.0 and v0.284.1 were never diff --git a/CONTEXT.md b/CONTEXT.md index f879545..48429c6 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,14 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-09-30 late evening (v0.284.0 + v0.284.1 — image retention, the install hold; v0.284.1 wires RemoveStack; v0.284.2 sees untagged digest-pulled images) +Last updated: 2026-10-01 (v0.285.0 — controller image retention, decision 56; R-751 nil-stack crash in the update clean-up) + +> **2026-10-01 — v0.285.0.** Operator ruling `09` §3 decision 56 (R-745): `stacks/controller_image_retention.go` keeps +> the running controller image + the previous (swap record `update-state.json` via `UpdateState.RecordedPrevious`; version +> order as fallback) + anything above the running version + non-version tags; deletes older/untagged by exact name/ID, +> 3 min after every start (main.go, after the one-time app sweep). Measured: the agent's swap rolls back to the RUNNING +> image (what `/etc/felhom-controller-image` named), never to a controller-supplied previous. R-751: nil-stack panic in +> `RetainImagesAfterUpdate` fixed; both retention seams are no-ops in the stacks tests (`TestMain`). > **2026-09-30 late — v0.284.0.** Operator rulings: `09` §3 decision 53 (R-736 A: keep running + previous image per > service, delete older, never an image a container/compose/record names) → `stacks/image_retention.go`, with a diff --git a/REUSE.md b/REUSE.md index 65a2092..ff51ac2 100644 --- a/REUSE.md +++ b/REUSE.md @@ -30,6 +30,7 @@ | `stacks.OpenSignupWindow` / `SignupBlocked` / `SetupGateProbe` · `web.ServeSignupClosed` (v0.281.0, decision 47) | controller/internal/stacks/signup_block.go · controller/internal/web/setup_gate.go | `(name)` | Sign-up closed at the app's own address once the gate opens; the household's 15-minute window; the press asks the probe | **The block goes up BEFORE the gate comes down** (a failed write keeps the gate closed). Never on an app this box did not gate | | `stacks.OpenInstallHold` / `installHoldTick` (v0.284.0, R-741) | controller/internal/stacks/install_hold.go | `(name, by)` / `()` | An `after_install` app held behind the setup gate's door until its known login is replaced | **Written before the first start**, like the gate; opens on `after_install` success or the household's "I changed it"; the door (`SetupGateHost`) reads holds first | | `stacks.RetainImagesAfterUpdate` / `RetainImagesAfterRemove` / `RunImageRetentionOnce` · seam `imageDocker` (v0.284.0, decision 53) | controller/internal/stacks/image_retention.go | `(name, previous)` / `(name, repos)` / `()` | Deletes an app's images older than its running + previous one | **The keep set is box-wide and read at delete time** (containers, installed composes, installed/previous records); exact id, never forced or pruned; skipped while any update runs; tests use the `imageDocker` seam, never Docker | +| `stacks.RetainControllerImages(ControllerImageRecord)` · `selfupdate.UpdateState.RecordedPrevious` (v0.285.0, decision 56) | controller/internal/stacks/controller_image_retention.go | `({Repo, Running, Previous})` | Deletes controller images older than the running + previous one | The previous comes from the SWAP RECORD (success onto the running version), version order only as the fallback; versions above the running one and non-version tags are kept; skipped while the controller swaps itself; same `imageDocker` seam | | `stacks.CloseSignupNow` / `CloseSignupOffered` / `applyNativeLock` (v0.282.0, decisions 47/49) | controller/internal/stacks/after_setup.go | `(name)` | The app's own sign-up switch after the setup; "close sign-up now" for an app installed before the rule | **Check the installed compose reads the variable** (an old install carries the old compose until its next update) — never record a lock that is not there. One run per app at a time (`nativeLockBusy`) | | `backup.judgeCopy` / `HollowCopies` / `SetHollowCopyNotify` (Part D, v0.279.0) | controller/internal/backup/hollow_watch.go | `(app, tier, unitDir)` | A RUNNING app whose newest copy holds no data → operator digest once/day + page sentence | Uses `unitCarriesData` (the manifest, never size); a stopped held app is never flagged | | `web.nightChain` (R-705, v0.279.0) | controller/internal/web/night_chain.go | `POST /api/debug/backup/night-chain` | The night's four legs now, in order | Refuses while any op/update/chain runs; the leg uses `RunUpdateLegNow` | diff --git a/controller/README.md b/controller/README.md index 176b8b5..2eed6cd 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1939,6 +1939,13 @@ that folder is never a dead end, and an install never runs into it silently (R-6 kept are every container's image, every installed compose's, and each installed app's running + `previous_images`. By exact id, never forced or pruned; no pass while any update runs; a one-time sweep at the first start after the release (marker `image-retention-v2.done` in the data dir). See `internal/stacks/image_retention.go`. +- **Controller image retention (v0.285.0, decision 56)** — 3 minutes after every start the box keeps the controller + image it runs and the one before it (the swap record `update-state.json` `previous_image` of a swap that ended on the + running version; without one, the highest version below the running one), every version above the running one (a + pulled target) and every non-version tag (`latest`, `-rc`); older and untagged controller images are deleted by exact + name/ID after the in-use check. Nothing while the controller swaps itself; registry tags are never touched. The + agent's own roll-back needs only the running image (it restores what `/etc/felhom-controller-image` named when the + swap began). See `internal/stacks/controller_image_retention.go`. - **The setup gate (v0.280.0, decision 46)** — `.felhom.yml` `setup_gate: true` + optional `setup_done_probe: {url, field, done}`. A FRESH install is closed to everyone but the household: the traefik file `/traefik/dynamic/setup-gate-.yml` is written BEFORE the first start (a failed write refuses the install) and diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index fd9d726..994e35c 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1696,6 +1696,14 @@ func main() { case <-time.After(3 * time.Minute): } stackMgr.RunImageRetentionOnce() + // decision 56 (R-745): the controller's own images — keep the running one and the one before it. At every + // start, 3 minutes in: a swap has ended by then (the agent's verify window is 90 s), so the image the agent + // would roll back to is the running one, and the one before it is in the swap record read here. + prevCtl := "" + if st, err := selfupdate.LoadState(cfg.Paths.DataDir); err == nil { + prevCtl = st.RecordedPrevious(Version) + } + _, _ = stackMgr.RetainControllerImages(stacks.ControllerImageRecord{Repo: cfg.SelfUpdate.Image, Running: Version, Previous: prevCtl}) }() // --- Initialize API router --- diff --git a/controller/internal/selfupdate/state.go b/controller/internal/selfupdate/state.go index fff69c9..054d2fe 100644 --- a/controller/internal/selfupdate/state.go +++ b/controller/internal/selfupdate/state.go @@ -71,3 +71,14 @@ func ClearState(dataDir string, logger *log.Logger) { } logger.Printf("[INFO] [selfupdate] Update state cleared") } + +// RecordedPrevious is the controller image the box ran before `running`, by the swap's own record (decision 56, +// R-745): the previous_image of an update that ended on the running version. "" when the record names none — the +// last swap failed, was to another version, or there is no record (a box set up by hand). Pinned by +// TestRecordedPrevious in state_test.go. +func (s *UpdateState) RecordedPrevious(running string) string { + if s == nil || s.Status != "success" || s.TargetVersion != running || s.PreviousVersion == running { + return "" + } + return s.PreviousImage +} diff --git a/controller/internal/selfupdate/state_test.go b/controller/internal/selfupdate/state_test.go new file mode 100644 index 0000000..6593e98 --- /dev/null +++ b/controller/internal/selfupdate/state_test.go @@ -0,0 +1,26 @@ +package selfupdate + +import "testing" + +// decision 56 (R-745): the controller's previous image comes from the swap record only when that record is a +// SUCCESS that ended on the running version. COMPANION RED-PROOF: drop the Status check → the failed-swap row fails. +func TestRecordedPrevious(t *testing.T) { + ok := &UpdateState{Status: "success", PreviousVersion: "0.284.2", PreviousImage: "r:0.284.2", TargetVersion: "0.285.0"} + cases := []struct { + name string + st *UpdateState + running string + want string + }{ + {"success onto the running version", ok, "0.285.0", "r:0.284.2"}, + {"no record", nil, "0.285.0", ""}, + {"the record is about another version", ok, "0.286.0", ""}, + {"a failed swap (rolled back: the box runs the 'previous')", &UpdateState{Status: "failed", PreviousVersion: "0.284.2", PreviousImage: "r:0.284.2", TargetVersion: "0.285.0"}, "0.285.0", ""}, + {"pending", &UpdateState{Status: "pending", PreviousImage: "r:0.284.2", TargetVersion: "0.285.0"}, "0.285.0", ""}, + } + for _, c := range cases { + if got := c.st.RecordedPrevious(c.running); got != c.want { + t.Errorf("%s: got %q, want %q", c.name, got, c.want) + } + } +} diff --git a/controller/internal/stacks/controller_image_retention.go b/controller/internal/stacks/controller_image_retention.go new file mode 100644 index 0000000..cac412c --- /dev/null +++ b/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 () 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 == "" || 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 + ":" + } + 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 +} diff --git a/controller/internal/stacks/controller_image_retention_test.go b/controller/internal/stacks/controller_image_retention_test.go new file mode 100644 index 0000000..ea5b6f0 --- /dev/null +++ b/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: "", 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) + } +} diff --git a/controller/internal/stacks/image_retention.go b/controller/internal/stacks/image_retention.go index 5f98fbd..b38f866 100644 --- a/controller/internal/stacks/image_retention.go +++ b/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) } diff --git a/controller/internal/stacks/r650_main_test.go b/controller/internal/stacks/r650_main_test.go index 5e626ac..e116101 100644 --- a/controller/internal/stacks/r650_main_test.go +++ b/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)) +}