From 943c51755081507d589333e93a5f78c15c469767 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Thu, 22 Oct 2026 21:25:48 +0200 Subject: [PATCH] Review fixes for 0.340.0: C5 a catch-up releases the whole-guest backup when its backup legs end (red-proved); C3 the R-1187 c refusal text fits an unhealthy app; C6 two comments; C8 README backup.volumes wording; unreleased --- CHANGELOG.md | 10 ++++++- controller/README.md | 5 ++-- controller/cmd/controller/main.go | 12 +++++---- controller/internal/appexport/restore.go | 6 +++-- controller/internal/nightchain/nightchain.go | 7 +++++ controller/internal/nightchain/r1189d_test.go | 26 +++++++++++++++++++ controller/internal/stacks/metadata.go | 2 +- 7 files changed, 57 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55883a34..4ab850f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,15 @@ meghajtóra készülő mentési másolat…"). The same night's review also moved the capped copy to `--mount` (a path the Docker daemon does not see is refused — exit 125, the in-process copy — instead of created empty, where `--delete` would empty the copy) and to the controller's image ID with `--pull never`; the copy container runs with `--no-healthcheck` (the image's - controller health check showed it as „starting" on the 9202 proof). Tests `TestStrayCopy_*` (backup, web, + controller health check showed it as „starting" on the 9202 proof). +- **Review fixes (the same night's fresh review of everything in 0.340.0, felhom.eu `REPORT-night-2026-10-23.md`).** + C5: a catch-up stops holding the whole-guest backup when its backup legs end — the after-catch-up off-site proof and + integrity check (R-1189 d) no longer keep `Running()` true, which could push that day's whole-guest backup out of its + window on a big store; `TestC5_TheWholeGuestBackupDoesNotWaitForTheChecks`, red-proved. C3: the R-1187 (c) refusal's + text now fits an app that runs but is unhealthy or restarting (IsStackRunning counts only running/degraded) — „leállt, + vagy hibásan indul … ha hibásan fut, távolítsd el"; the import path for those states stays a row (R-1187). C6: two + main.go comments no longer say a box off at 05:30–06:00 always gets the proof made up (only with a missed backup leg). + C8: README — `backup.volumes` excluded leaves the app-data backups, not the `.fab` export or the whole-guest snapshot. Tests `TestStrayCopy_*` (backup, web, wiring; Docker-free seams), each red-proved; 9202 proof in felhom.eu `audits/night-2026-10-23/P1-straycopy/`. ## Unreleased — the second-drive copy no longer squeezes the apps' memory (R-1214), 2026-10-22 night diff --git a/controller/README.md b/controller/README.md index ccd22513..c66de565 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1399,10 +1399,11 @@ retired in v0.126.0 when the moved blocks were legitimately rewritten onto the s contain no symlink (`tier2CappedPath`, `filepath.EvalSymlinks(p) == p`: a link could point the mount elsewhere). A docker start error (exit 125) falls back to plain rsync; any other failure is returned. Measured on 9202 (4 GB, 1.8 GB held, 6 GB copy): full stall 5.7 s → 0.37 s. Tests `r1214_capped_copy_test.go`. -- **A template may leave a named volume out of every backup (unreleased, rides 0.340.0; R-49, operator D21 = B).** +- **A template may leave a named volume out of the app-data backups (unreleased, rides 0.340.0; R-49, operator D21 = B).** `.felhom.yml` `backup.volumes: [{name: , class: excluded}]` — every named volume is dumped except `_` (`appbackup.ExcludedDockerVolumes` → `backup.Manager.dumpableVolumes`), so it leaves Tier 1, Tier 2 - and off-site at once and the stale clean-up removes the old tar. Only `excluded` acts; an undeclared name is ignored + and off-site at once and the stale clean-up removes the old tar. NOT left out: the household's `.fab` export (it still + carries the volume) and the whole-guest PBS snapshot (it holds the guest's disk). Only `excluded` acts; an undeclared name is ignored and logged; a list that would exclude ALL of an app's named volumes is refused whole. First user: Immich's `immich_ml_cache` (model weights; Immich re-downloads them). Design: `07` §6.2. diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index 9ffa19e7..59eff6cf 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1564,8 +1564,10 @@ func main() { notifier.NotifyBackupCatchUp(missedAt.In(budapestLoc()).Format("15:04")) }, // R-1189 (d): the off-site proof (05:30) and the integrity check (06:00) are daily jobs the scheduler - // never runs late; a box that is off at those hours comes back through this catch-up, so they run - // here — the proof picks one due snapshot, the integrity check runs only when it is due (max age). + // never runs late; a box that also MISSED a backup leg comes back through this catch-up, so they run + // here — the proof picks one due snapshot, the integrity check runs only when it is due (max age). A box + // that was on for the backup legs but off at 05:30–06:00 gets no catch-up: its integrity check catches + // up at the next 06:00 (due-ness); that day's proof is not made up (review 2026-10-22 night, C6). After: func(ctx context.Context) { runOffsiteProof(ctx, backupMgr, notifier, logger) runOffsiteIntegrityCheck(ctx, backupMgr, notifier, logger, false) @@ -1612,8 +1614,8 @@ func main() { // // DAILY JOB, WEEKLY BEHAVIOUR, and that is the point. It asks "is the last successful check // older than the max age?" rather than "is it Sunday?", so a box that was switched off on its - // check day is checked the next day it is on AT 06:00 — or, if it is off at that hour, at its next - // catch-up (R-1189 d: the catch-up's After hook runs this check and the proof). R-341 is exactly the other shape: a dated check + // check day is checked the next day it is on AT 06:00 — or at its next catch-up, when it also missed a + // backup leg (R-1189 d: the catch-up's After hook runs this check and the proof). R-341 is exactly the other shape: a dated check // quietly missed and never caught up. No `Weekly` primitive was added to the scheduler — // due-ness is smaller, catches up, and is what R-86 already chose for restore-tests. // @@ -3137,7 +3139,7 @@ func (a *stackAdapter) GetDockerVolumes(name string) []string { return backup.ResolveDockerVolumeNames(s.ComposePath) } -// GetExcludedDockerVolumes (R-49, D21 = B): the named volumes the template excludes from every backup; an ignored +// GetExcludedDockerVolumes (R-49, D21 = B): the named volumes the template excludes from every app-data backup (not the .fab export); an ignored // template entry is logged, never fatal (backup/volume_exclusion.go). func (a *stackAdapter) GetExcludedDockerVolumes(name string) []string { ex, ignored := a.mgr.ExcludedDockerVolumes(name) diff --git a/controller/internal/appexport/restore.go b/controller/internal/appexport/restore.go index 900cf9ab..04781fb5 100644 --- a/controller/internal/appexport/restore.go +++ b/controller/internal/appexport/restore.go @@ -362,11 +362,13 @@ func (e *Exporter) executeImport(req ImportRequest, job *Job) { // NOT running used to „overwrite config in place" — its volumes were never removed, and `docker cp` only adds and // overwrites, so the result was the old data MERGED with the bundle's (a stale SQLite -wal beside the bundle's .db // is a corruption shape), reported as success. Only a running app's volumes are removed first. So this import is - // refused here, before anything is written; the household starts the app (or removes it) and imports again. + // refused here, before anything is written; the household starts the app (or removes it) and imports again. „Not + // running" is IsStackRunning's view: running or degraded only, so an unhealthy or restarting app is refused too — + // the text says so and names removal (review 2026-10-22 night, C3; the import path for those states is R-1187). // Pinned by TestR1187c_AStoppedAppIsNotImportedOver. if isDeployed && !isRunning { e.logger.Printf("[WARN] Import: refused — %s is installed but not running; importing over it would merge the old data with the bundle's (R-1187 c)", manifest.AppName) - e.failJob(job, step, "Az alkalmazás telepítve van, de nem fut, ezért a csomagot nem töltjük vissza: a régi adatai összekeveredhetnének a csomagéval. Az alkalmazás érintetlen. Indítsd el (vagy távolítsd el), és próbáld újra.") + e.failJob(job, step, "Az alkalmazás telepítve van, de nem fut rendben (leállt, vagy hibásan indul), ezért a csomagot nem töltjük vissza: a régi adatai összekeveredhetnének a csomagéval. Az alkalmazás érintetlen. Ha leállt, indítsd el; ha hibásan fut, távolítsd el. Utána próbáld újra.") return } diff --git a/controller/internal/nightchain/nightchain.go b/controller/internal/nightchain/nightchain.go index 16e81891..de6b8558 100644 --- a/controller/internal/nightchain/nightchain.go +++ b/controller/internal/nightchain/nightchain.go @@ -340,6 +340,13 @@ func (c *CatchUp) run(ctx context.Context, why string) { names = append(names, string(leg)) } c.Ledger.markCatchUp(c.now()) + // The backup legs are done: the whole-guest backup no longer waits for this catch-up (review 2026-10-22 night, C5). + // The checks below read only the off-site store, and on a big store they take long — held, they could push that + // day's whole-guest backup out of its window. `pending` stays set until run() returns (no second catch-up). + // Pinned by TestC5_TheWholeGuestBackupDoesNotWaitForTheChecks. + c.mu.Lock() + c.running = false + c.mu.Unlock() if c.After != nil { c.Logger.Printf("[INFO] [catch-up] running the after-catch-up checks (the off-site proof and the due-gated integrity check, R-1189 d)") c.After(ctx) diff --git a/controller/internal/nightchain/r1189d_test.go b/controller/internal/nightchain/r1189d_test.go index b328a154..24d5c539 100644 --- a/controller/internal/nightchain/r1189d_test.go +++ b/controller/internal/nightchain/r1189d_test.go @@ -43,3 +43,29 @@ func TestR1189d_TheChecksRunAfterTheCatchUpLegs(t *testing.T) { t.Fatal("no catch-up ran, so the after-catch-up checks must not run") } } + +// C5 (review 2026-10-22 night): while the after-catch-up checks run, the catch-up no longer reports Running() — the +// scheduled whole-guest backup (quiesce.go, R-871) waits only for the backup legs, not for the off-site proof and the +// integrity check, which take long on a big store. RED-PROOF: with the `c.running = false` before After removed, +// this fails — seen failing 2026-10-22 night. +func TestC5_TheWholeGuestBackupDoesNotWaitForTheChecks(t *testing.T) { + h := newHarness(t, at(1, 12, 0), at(5, 9, 0)) + h.l.MarkEnded(LegDBDump, at(4, 2, 31)) + h.l.MarkEnded(LegTier2, at(4, 3, 31)) + h.l.MarkEnded(LegOffsite, at(4, 4, 20)) + var mu sync.Mutex + runningDuringAfter := []bool{} + h.c.After = func(context.Context) { + r := h.c.Running() + mu.Lock() + runningDuringAfter = append(runningDuringAfter, r) + mu.Unlock() + } + h.c.Evaluate(context.Background(), "start") + h.wait(t) + mu.Lock() + defer mu.Unlock() + if len(runningDuringAfter) != 1 || runningDuringAfter[0] { + t.Fatalf("Running() during the after-catch-up checks = %v, want [false]", runningDuringAfter) + } +} diff --git a/controller/internal/stacks/metadata.go b/controller/internal/stacks/metadata.go index e4beabd6..89542b3b 100644 --- a/controller/internal/stacks/metadata.go +++ b/controller/internal/stacks/metadata.go @@ -597,7 +597,7 @@ func (m *Manager) ClassifiedBinds(name string) ([]appbackup.ClassifiedBind, bool return appbackup.ClassifyBinds(meta.Backup, binds) } -// ExcludedDockerVolumes returns the Docker names of the stack's named volumes its template excludes from every backup +// ExcludedDockerVolumes returns the Docker names of the stack's named volumes its template excludes from every app-data backup // (R-49, D21 = B; appbackup.ExcludedDockerVolumes), and a sentence per template entry that was ignored. func (m *Manager) ExcludedDockerVolumes(name string) ([]string, []string) { stack, ok := m.GetStack(name)