diff --git a/CHANGELOG.md b/CHANGELOG.md index 5574a34..4cccc1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,43 @@ +## v0.244.0 — the backup page stops promising what it does not hold, and a restore refuses to lie (2026-09-16, R-537 / R-538 / R-536) + +**MinAgent: 0.131.0** (unchanged — nothing here needs a newer agent) + +- **R-537 — the app-backup page labelled a Tier-1 unit „DB + Konfig + Adatok" and printed the app's + data-drive size beside it, over a unit that holds no copy of those files.** The label was computed + from the APP's shape (`HasHDDData || HasVolumeData`) and rendered on all three tier rows, so one + string stood for three tiers that capture different things. It is now computed per tier from what + the tier actually carries: Tier 1 says „Adatok" only when the app's data really is inside the + volumes the unit captured, and a class-A app gets one sentence saying where its files ARE + protected. **The design is unchanged** (07-backup-architecture §6.1/§6.2: a Tier-1 unit has no + file-copy step; the file legs of calibre-web, immich, nextcloud and paperless-ngx are carried by + Tier 2 and Tier 3) — what changed is that the page now says so. Measured on a fresh box + 2026-09-16: five photos in Nextcloud, in no backup, with the page saying they were. + Red-proof: restore the old app-shaped label → `TestAppBackupRows_Tier1LabelDoesNotClaimFilesItCannotHold` + fails at „the Tier-1 label claims it holds the app's data". +- **R-538 — a unit restore replayed a database over files it did not have, reported success, and + destroyed the app's own wastebasket on the way.** `RestoreFromRecoveryUnitAt` now REFUSES before + anything is touched when the app's files live on the data drive, names the route that can return + them (the off-site wizard's „Teljes visszaállítás (fájlok + adatbázis)", or the second drive's + „Fájlok visszaállítása"), and says plainly when there is no copy at all. The explicit + database-and-settings-only path is a separately-worded second step + (`UnitRestoreOptions{AcceptMissingFiles}`), never a sibling control. The refusal runs before the + stack is stopped, because the measured harm included the trash going unreachable. + Red-proof: disable the guard → `TestUnitRestore_RefusesWhenTheUnitCannotHoldTheFiles` fails at + „a restore that cannot return the files must refuse". +- **R-536 — the hub was told „Alkalmazás telepítve" when the deploy was merely ACCEPTED.** The API + now emits `app_deploy_started` beside its 202, and `app_deployed` is emitted from the async path's + own end (`stacks.SetDeployDoneHook`), with `app_deploy_failed` (warning) when it ends badly — + which used to be silence. Measured 2026-09-16: mealie was recorded as installed in the same second + its deploy was accepted, then killed 5 s in, and ended `not_deployed` with nothing correcting the + event. The accept-time `app.yaml` is deliberately NOT deleted on failure: it is the crash-safe + record written with `Deployed:false` and it carries the settings the customer typed. + Red-proofs: put the old call back → `TestDeployAcceptance_DoesNotClaimTheAppIsInstalled` fails; + remove the success-path hook → `TestDeployDoneHook_FiresAtTheEndAndSaysWhichEndItWas` fails at + „the deploy ended and nothing was told about it". +- Requires hub **v0.116.0**, which registers `app_deploy_started` / `app_deploy_failed` in both + `allowedEventTypes` and `customerMessages`; against an older hub those two POSTs 400 and the + events are simply absent (`app_deployed` keeps working). + ## Unreleased — after v0.243.0 (2026-09-15) - **R-517 follow-up — the whole-system tile printed „0 B" for a backup whose size is unknown.** diff --git a/REPORT.md b/REPORT.md index a671b72..c7321e7 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,36 +1,57 @@ -# REPORT — controller v0.243.0: a real file-manager password, a truthful backup page, no stop for a missing tier (2026-09-15) +# REPORT — controller v0.244.0: the backup page stops promising what it does not hold -Task: *before the volunteer — the big night's P1 fixes*, Parts B, C, E.2 (controller half). **MinAgent: 0.131.0.** -Architecture: `07-backup-architecture.md` §6, `02-controller-module-map.md` (settings after install). +**2026-09-16.** Three defects the 2026-09-16 drill measured on a fresh box, all in the same family: +the product said a thing that was not true about a customer's data. -## R-513 — FileBrowser password -Measured first (B.1): the config key / env var sets the password but re-applies it on every start (a hand-set password -is overwritten); the API (`PUT /api/users?id=…` + `X-Password: `) changes it once. Shipped the API path for -fresh and existing boxes: probe admin/admin → 200: generate, set, verify new=200 and admin=401, store encrypted → -`generated`; 401 → `operator`; unreachable → nothing recorded. App page shows user `admin` + reveal (R-254 rules). -**Live:** 9202 (default) → generated, reveal 200 (16 chars), revealed=200 / admin=401 / wrong=401. 9201 (hand-set) → -first probe failed on DNS before the network join (retried), then `operator` at 08:52:10Z, untouched. Public -`files.enkisfelhom.hu` admin → 401. *One invalid run first (empty credential read) — marked in the evidence.* +## What shipped -## R-517 / R-518 — backup page and tier skip -Per tier: newest success, failed attempt under it, „nincs beállítva" for absent storage; „Naprakész" and the remote tick -from successes only; a tier with absent storage is skipped before anything stops (`backup_tier_skipped`). Button copy -tells the real downtime. **Live on 9201:** local and PBS rows „✓ … Naprakész", remote tick on a real PBS success. Found -live: the top card printed „0 B" for a size read back from storage → fixed to „–" on `main` (d3eacbb), **unreleased**. -Not live-measured: the absent-storage and failed-PBS states (unit-proven; making them on a demo box means removing its -PBS storage). +**R-537 — the label is now per tier.** `BackupContents` was one string, computed from the app's shape +(`HasHDDData || HasVolumeData → "Adatok"`) and rendered on the Tier-1, Tier-2 and Tier-3 rows alike. +One string cannot be true for three tiers that capture different things: a Tier-1 unit holds compose ++ app.yaml + DB dumps + volume tars and has **no file-copy step at all** (`CaptureRecoveryUnit`; +`RecoveryManifest` has no field for one), so for the four class-A apps the customer's own files are +carried by Tier 2 and Tier 3 and by nothing else. The row now carries `Tier1Contents`, +`Tier23Contents` and `DriveFilesNote`, and the template renders the right one per row. -## R-514 — OOM visibility -`State.OOMKilled` read for running app containers → dashboard „Memória elfogyott" + `app_oom`. **Not proven live:** -in the LXC guest Docker reported neither a restart-OOM nor a child-process OOM (R-528). The task's tag text -„… — újraindítva" was not used: the controller does not restart the app. +**R-538 — the restore refuses instead of lying.** `RestoreFromRecoveryUnitAt` now returns +`*ErrUnitLacksFileLegs` **before the lock, before the stack is stopped and before any volume is +replaced**, when the app declares drive-side file legs the unit cannot hold. The web handler turns +that into a Hungarian sentence naming the route that CAN return the files — the off-site wizard's +„Teljes visszaállítás (fájlok + adatbázis)", or the second drive's „Fájlok visszaállítása" — and says +plainly when no copy exists. `UnitRestoreOptions{AcceptMissingFiles}` is the explicit, separately +worded second step; it is a per-call argument and never a field on the Manager. -## Red-proofs -PUT removed → admin/admin still logs in; operator branch removed → page does not say who set it; attempt-as-success → -size lost; skip disabled → manual run starts felhom-pbs, scheduled run stops an app; OOM filter inverted → killed worker -hidden; size flag removed → „would print a size it does not know". +*Why the refusal runs that early:* in the measured failure the replayed database also stopped +referencing the app's own wastebasket, which still held every byte on the drive. A refusal that has +already stopped the app would have destroyed the customer's last route while declining to help. -## Delivery -Image `felhom-controller:0.243.0` built from `843b319`. Hub floor for **demo-hp only** 0.243.0 + declared MinAgent -0.131.0 → `managed floor SERVED … from declared`; 9201 on 0.243.0 in 19 s. Scratch 9202 set by hand (its disposition). -demo-felhom not moved (its agent is 0.130.0, the floor would hold). Full suite green before each commit. +**R-536 — „telepítve" now means installed.** The API emits `app_deploy_started` beside its 202; +`app_deployed` moved to the async path's own end via `stacks.SetDeployDoneHook`, and +`app_deploy_failed` (warning) replaces the silence an interrupted install used to get. The +accept-time `app.yaml` is deliberately kept on failure: it is the crash-safe record written with +`Deployed:false`, it holds the settings the customer typed, and the state every surface reads is +`not_deployed`. + +Also folded in: the pending „0 B" whole-system tile fix (R-517 follow-up). + +## Red-proofs — each seen failing, then passing + +| fix | break | what failed | +|---|---|---| +| R-537 label | restore the app-shaped label | „the Tier-1 label claims it holds the app's data: `Konfig + Adatok`" | +| R-538 refusal | disable the guard | „a restore that cannot return the files must refuse; got err=``" | +| R-536 accept-time | put `NotifyAppDeployed` back beside the 202 | „the deploy handler announces an INSTALLED app at accept time" | +| R-536 completion | remove the success-path hook | „the deploy ended and nothing was told about it" | + +Each test also carries a negative control: an app whose data really is in the captured volumes keeps +its „Adatok" and is not refused. + +## Gates + +`go build ./... && go vet ./... && go test ./...` — green. `controller_gates.py --fast` — all 15 OK. + +## Requires + +Hub **v0.116.0**, which registers `app_deploy_started` / `app_deploy_failed` in `allowedEventTypes` +and `customerMessages`. Against an older hub those two POSTs 400 and the events are simply absent; +`app_deployed` keeps working. MinAgent unchanged at **0.131.0**. diff --git a/controller/README.md b/controller/README.md index f1b56c8..b717b49 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1636,7 +1636,7 @@ Unified per-app status table with expandable rows showing **per-tier** backup st Every app starts as yellow (1 tier only). Green requires Tier 2 configured with successful backup. **Per-app backup tiers (3 rows per app):** -- **1. mentes** (Tier 1, always present) — Auto badge + "helyi" + last run + contents (e.g., "DB + Konfig + Adatok") +- **1. mentes** (Tier 1, always present) — Auto badge + "helyi" + last run + contents. **The contents label is PER TIER (v0.244.0, R-537):** it describes what that tier actually captured, not what the app is shaped like. - **2. mentes** (Tier 2, configurable for ALL apps) — one of: - Configured: method (rsync/restic) + destination + schedule + last run + status + contents + browsable indicator (folder icon for rsync) + action buttons - Not configured: "1. mentes auto" + "Nincs 2. masolat" + settings link @@ -1645,10 +1645,10 @@ Every app starts as yellow (1 tier only). Green requires Tier 2 configured with ("Kulcsletetre var"), `active` (status badge + "restic -> " + relative last-run) **Backup contents per app** (shown per tier): -- Apps with DB + HDD: "DB + Konfig + Adatok" -- Apps with Docker volumes (no HDD): "Konfig + DB + Adatok" or "Konfig + Adatok" +- Apps whose files live on the data drive (class A: calibre-web, immich, nextcloud, paperless-ngx): Tier 1 reads **"DB + Konfig"** — a Tier-1 unit has no file-copy step, so it does not hold them — and a sentence under the row says the files are protected by the off-site copy (and a second drive). Tier 2/3 read "DB + Konfig + Adatok", because those tiers DO carry the file legs. +- Apps whose data is entirely in Docker volumes (45 of 53 templates): "Konfig + DB + Adatok" or "Konfig + Adatok" on every tier — the unit really does hold their data. - Apps with DB only: "DB + Konfig" -- Apps with HDD, no DB: "Konfig + Adatok" +- **The restore refuses rather than lying (v0.244.0, R-538):** „Visszaállítás indítása" on a unit that cannot return an app's drive-side files is REFUSED before anything is stopped, naming the route that can (the off-site „Teljes visszaállítás (fájlok + adatbázis)", or the second drive's „Fájlok visszaállítása"), and saying plainly when no copy exists. A database-and-settings-only restore is a separately-worded second step. - Apps with neither: "Konfig" **Deploy page** shows cross-drive (Tier 2) configuration form for **all deployed apps**, diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index be68663..cdc752a 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -1591,6 +1591,22 @@ func main() { // Migration done-hook: a decommission-initiated migration finalizes the source decommission on // success (soft-mark + agent). Wire it before RecoverMigration so a resumed one still finalizes. stackMgr.SetMigrationDoneHook(webServer.OnMigrationDone) + // R-536: „Alkalmazás telepítve" is sent when the async deploy ACTUALLY finishes, and a deploy + // that ends badly now says so instead of being silence. The accept-time event is + // `app_deploy_started`, emitted by the API router beside its 202. + if notifier != nil { + stackMgr.SetDeployDoneHook(func(name string, ok bool, detail string) { + display := name + if s, found := stackMgr.GetStack(name); found && s.Meta.DisplayName != "" { + display = s.Meta.DisplayName + } + if ok { + notifier.NotifyAppDeployed(name, display) + return + } + notifier.NotifyAppDeployFailed(name, display, detail) + }) + } stackMgr.RecoverMigration(ctx) webServer.SetEncryptionKey(encKey) webServer.SetAppExporter(appExporter) diff --git a/controller/internal/api/r536_accept_time_event_test.go b/controller/internal/api/r536_accept_time_event_test.go new file mode 100644 index 0000000..ed883f6 --- /dev/null +++ b/controller/internal/api/r536_accept_time_event_test.go @@ -0,0 +1,36 @@ +package api + +import ( + "os" + "strings" + "testing" +) + +// R-536 — the ACCEPT-time path must not claim the app is installed. +// +// The router holds a concrete *notify.Notifier, so there is no seam to fake here; the claim is +// therefore asserted where it actually lives — in the source of the deploy handler. This is the same +// shape as the stacks package's AST test, and it exists for the same reason: the defect was not a +// wrong value, it was a call in the wrong place. +// +// What it pins: the deploy handler (which writes 202 „Telepítés elindítva" and returns, while the +// compose up runs afterwards) may announce that the install STARTED, and may not announce that it +// finished. The finishing half is stacks.deployDoneHook → main.go → NotifyAppDeployed, pinned by +// TestDeployDoneHook_FiresAtTheEndAndSaysWhichEndItWas. +// +// Red-proof: put `r.notifier.NotifyAppDeployed(name, displayName)` back beside the 202 → this fails. +func TestDeployAcceptance_DoesNotClaimTheAppIsInstalled(t *testing.T) { + src, err := os.ReadFile("router.go") + if err != nil { + t.Fatal(err) + } + body := string(src) + + if strings.Contains(body, "NotifyAppDeployed(") { + t.Fatal("the deploy handler announces an INSTALLED app at accept time — the install has not run yet; " + + "an interrupted deploy then stands on the timeline as a completed one (measured 2026-09-16, mealie)") + } + if !strings.Contains(body, "NotifyAppDeployStarted(") { + t.Fatal("the acceptance is no longer recorded at all — the timeline loses the fact that the customer asked for this app") + } +} diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index bd7999d..9e878cf 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -498,7 +498,10 @@ func (r *Router) deployStack(w http.ResponseWriter, req *http.Request, name stri if s, ok := r.stackMgr.GetStack(name); ok && s.Meta.DisplayName != "" { displayName = s.Meta.DisplayName } - r.notifier.NotifyAppDeployed(name, displayName) + // R-536: this is the ACCEPTANCE, not the installation. `app_deployed` moved to the async + // path's own end (stacks.deployDoneHook → Router.OnDeployDone), because the deploy runs after + // this returns and an interrupted one used to leave a completed-install record behind. + r.notifier.NotifyAppDeployStarted(name, displayName) } // Re-sync geo rules (new hostname may need to be added) diff --git a/controller/internal/backup/file_legs.go b/controller/internal/backup/file_legs.go new file mode 100644 index 0000000..b35862f --- /dev/null +++ b/controller/internal/backup/file_legs.go @@ -0,0 +1,56 @@ +package backup + +import ( + "strings" + + "gitea.dooplex.hu/admin/felhom-controller/internal/appbackup" +) + +// DeclaredDriveFileLegs answers ONE question, for the label and for the restore guard alike: which +// of this app's own files live on the customer's DATA DRIVE rather than inside a Docker volume? +// +// R-537 / R-538 (measured 2026-09-16 on a fresh box). A Tier-1 recovery unit captures compose + +// app.yaml + the DB dumps and volume tars that already exist beside it — `CaptureRecoveryUnit` has +// no file-copy step at all, and `RecoveryManifest` has no field to record one. The whole-guest tiers +// do not carry them either (`mp8 /mnt/felhom-drives` is a bind mount and vzdump logs +// "excluding bind mount point mp8 … (not a volume)"). So for the four class-A apps the drive-side +// paths are carried by Tier 2 and Tier 3 ONLY — which is the design (07-backup-architecture §6.2), +// and is exactly why a page that says „Adatok" over a Tier-1 unit, or a restore that replays a +// database over files it does not have, is a lie rather than a design choice. +// +// It returns the DECLARED mandatory paths, resolved but deliberately NOT stat-filtered. The filter +// belongs to a capture (a declared path that is missing on disk is a capture gap, and +// `offboxCaptureSet` warns about it there). Here the question is what the app CLAIMS to keep on the +// drive, and an empty folder the customer has not filled yet must still count — otherwise the label +// tells the truth today and starts lying the moment they use the app. +// +// Empty for: no stack provider, a legacy app with no `backup:` block (nothing declares a namespace +// path — all 45 class-B apps), or an app with no resolvable HDD_PATH (undeployed). +func (m *Manager) DeclaredDriveFileLegs(stack string) []string { + if m.stackProvider == nil { + return nil + } + binds, has := m.stackProvider.GetStackClassifiedBinds(stack) + if !has { + return nil + } + hdd := strings.TrimSpace(m.stackProvider.GetStackHDDPath(stack)) + if hdd == "" { + return nil + } + cs := appbackup.ComputeCaptureSet(binds, has, appbackup.TierOffsite, m.namespaceRoot(hdd), m.stackProvider.GetImportRoot()) + out := make([]string, 0, len(cs.Paths)) + for _, p := range cs.Paths { + out = append(out, p.Abs) + } + if len(out) == 0 { + return nil + } + return out +} + +// HasDriveFileLegs is DeclaredDriveFileLegs as a predicate, for the surfaces that only need the +// yes/no. Kept beside it so the two can never disagree. +func (m *Manager) HasDriveFileLegs(stack string) bool { + return len(m.DeclaredDriveFileLegs(stack)) > 0 +} diff --git a/controller/internal/backup/r538_unit_restore_refusal_test.go b/controller/internal/backup/r538_unit_restore_refusal_test.go new file mode 100644 index 0000000..595e42d --- /dev/null +++ b/controller/internal/backup/r538_unit_restore_refusal_test.go @@ -0,0 +1,78 @@ +package backup + +import ( + "errors" + "testing" +) + +// R-538 — a unit restore must REFUSE when the unit holds no copy of the app's files. +// +// The defect this pins, measured live on 2026-09-16: five photos were put into Nextcloud, the +// customer pressed „Visszaállítás indítása" on the Tier-1 unit, and the restore replayed three +// volume tars and a database dump over an app whose files live on the data drive. It reported +// „3 adatkötet és az adatbázis visszaállítva", and afterwards the folder listed all five photos and +// none of them opened — the replayed database referenced files that were never captured, and it had +// also stopped referencing the app's own wastebasket, which still held every byte. +// +// The assertion is the CONSEQUENCE, not the mechanism: the call returns the refusal and the app is +// left alone. Red-proof: delete the guard in RestoreFromRecoveryUnitAtWith → this test fails at +// "a restore that cannot return the files must refuse". +func TestUnitRestore_RefusesWhenTheUnitCannotHoldTheFiles(t *testing.T) { + drive := t.TempDir() + m, _, prov := classifiedOffboxManager(t, drive) + + // A class-A app: it declares a MANDATORY bind under the drive, which is where its files live and + // which a Tier-1 unit structurally cannot capture. + prov.hdd["nextcloud"] = drive + prov.binds["nextcloud"] = []ClassifiedBind{mandatoryHDD("appdata/nextcloud")} + prov.has["nextcloud"] = true + mkUnit(t, drive, "nextcloud") + + _, err := m.RestoreFromRecoveryUnitAt("nextcloud", RecoveryUnitPath(drive, "nextcloud")) + var refusal *ErrUnitLacksFileLegs + if !errors.As(err, &refusal) { + t.Fatalf("a restore that cannot return the files must refuse; got err=%v", err) + } + if refusal.Stack != "nextcloud" || len(refusal.Paths) == 0 { + t.Fatalf("the refusal must name the app and the paths it cannot return: %+v", refusal) + } + + // NEGATIVE CONTROL, and it is the half that keeps the guard from being over-broad: an app that + // declares no drive-side files (all 45 class-B templates, whose data IS in the volumes the unit + // captured) must NOT be refused. If this ever starts refusing, the guard has stopped asking about + // the unit and started asking about nothing in particular. + prov.hdd["privatebin"] = drive + prov.has["privatebin"] = false + mkUnit(t, drive, "privatebin") + _, err = m.RestoreFromRecoveryUnitAt("privatebin", RecoveryUnitPath(drive, "privatebin")) + if errors.As(err, &refusal) { + t.Fatalf("an app with no drive-side files must not be refused: %v", err) + } + + // The explicit second step („csak az adatbázist és a beállításokat") passes the guard. It may + // still fail further down for unrelated fixture reasons — what is asserted is only that consent + // is what the guard consults. + _, err = m.RestoreFromRecoveryUnitAtWith("nextcloud", RecoveryUnitPath(drive, "nextcloud"), UnitRestoreOptions{AcceptMissingFiles: true}) + if errors.As(err, &refusal) { + t.Fatalf("explicit consent must pass the guard, not be refused by it: %v", err) + } +} + +// DeclaredDriveFileLegs is the ONE predicate the label (R-537) and the refusal (R-538) share. A +// second copy of this question is how a page and a guard drift apart, so it is pinned here too. +func TestDeclaredDriveFileLegs_IsAboutDeclarationNotExistence(t *testing.T) { + drive := t.TempDir() + m, _, prov := classifiedOffboxManager(t, drive) + prov.hdd["nextcloud"] = drive + prov.binds["nextcloud"] = []ClassifiedBind{mandatoryHDD("appdata/nextcloud")} + prov.has["nextcloud"] = true + + // The folder does NOT exist on disk in this fixture. It must still count: a label that tells the + // truth only until the customer starts using the app is not telling the truth. + if !m.HasDriveFileLegs("nextcloud") { + t.Fatal("a declared mandatory drive path must count even before the customer has put anything in it") + } + if got := m.DeclaredDriveFileLegs("unknown-app"); got != nil { + t.Fatalf("an app the provider does not know has no declared legs; got %v", got) + } +} diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index c7879da..6be7bb0 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -223,11 +223,61 @@ func (m *Manager) primaryUnitDirFor(stackName string) string { // start → replay → start, R-47), the secret reconciliation with unit-over-guest precedence and the // fail-closed data-key gate, and the no-unit fallback to RestoreApp with its CountsUnknown handling. func (m *Manager) RestoreFromRecoveryUnitAt(stackName, unitDir string) (UnitRestoreResult, error) { + return m.RestoreFromRecoveryUnitAtWith(stackName, unitDir, UnitRestoreOptions{}) +} + +// UnitRestoreOptions carries the caller's EXPLICIT consent for a restore this package would +// otherwise refuse. It exists because of R-538, and it has exactly one member for now. +type UnitRestoreOptions struct { + // AcceptMissingFiles lets a unit restore proceed for an app whose own files live on the data + // drive and are therefore NOT in the unit. The default — zero value, every existing caller — is + // to REFUSE, because the run that produced this option replayed a database over files that were + // never captured, reported „3 adatkötet és az adatbázis visszaállítva", and left Nextcloud + // listing five photos that returned `Sabre\DAV\Exception\NotFound`. + // + // It is a per-call argument and never a field on the Manager: a consent that outlives the act it + // was given for is not consent. + AcceptMissingFiles bool +} + +// ErrUnitLacksFileLegs is the refusal R-538 asks for. It names the app and the paths that are NOT in +// the unit, so the caller can build an honest sentence without re-deriving anything. +type ErrUnitLacksFileLegs struct { + Stack string + Paths []string +} + +func (e *ErrUnitLacksFileLegs) Error() string { + return fmt.Sprintf("%s: the recovery unit carries no copy of the app's files on the data drive (%s) — refusing to replay the database over them", + e.Stack, strings.Join(e.Paths, ", ")) +} + +// RestoreFromRecoveryUnitAtWith is RestoreFromRecoveryUnitAt with the caller's explicit consent +// flags. See UnitRestoreOptions. +func (m *Manager) RestoreFromRecoveryUnitAtWith(stackName, unitDir string, opt UnitRestoreOptions) (UnitRestoreResult, error) { var res UnitRestoreResult if m.stackProvider == nil { return res, fmt.Errorf("stack provider not configured") } + // R-538 — REFUSE BEFORE ANYTHING IS TOUCHED. This runs before the lock, before the stack is + // stopped and before a single volume is replaced, because the measured harm was not only the + // missing files: the replayed database also stopped referencing the app's OWN wastebasket, which + // still held every byte on the drive. A refusal that has already stopped the app has destroyed + // the customer's last route while declining to help them. + // + // The condition is about the UNIT, not the app class: a unit structurally cannot hold these + // paths (`CaptureRecoveryUnit` has no file-copy step, `RecoveryManifest` no field for one), and + // that is true of the Tier-2 mirror of a unit as well — Tier 2's file half is a separate action + // („Fájlok visszaállítása", RestoreTier2Files), which is exactly what the caller should offer. + if !opt.AcceptMissingFiles { + if legs := m.DeclaredDriveFileLegs(stackName); len(legs) > 0 { + m.logger.Printf("[WARN] [backup] unit restore REFUSED for %s: the unit carries no file leg; %d drive path(s) would be left as they are: %s", + stackName, len(legs), strings.Join(legs, ", ")) + return res, &ErrUnitLacksFileLegs{Stack: stackName, Paths: legs} + } + } + m.mu.Lock() if m.running { m.mu.Unlock() diff --git a/controller/internal/notify/notifier.go b/controller/internal/notify/notifier.go index 717d596..36500cd 100644 --- a/controller/internal/notify/notifier.go +++ b/controller/internal/notify/notifier.go @@ -515,12 +515,40 @@ type EndpointDriftDetails struct { } // NotifyAppDeployed sends an app deployment event. +// +// R-536: it is called when the deploy COMPLETES (the compose up succeeded and the app's durable +// state was written), never when the request was merely accepted. It used to fire beside the 202, +// so an install that was interrupted five seconds later still stood on the hub's timeline as +// „Alkalmazás telepítve" forever — measured 2026-09-16 with mealie, which ended `not_deployed`. func (n *Notifier) NotifyAppDeployed(stackName, displayName string) { n.PushEvent("app_deployed", "info", fmt.Sprintf("Alkalmazás telepítve: %s", displayName), AppDetails{StackName: stackName, DisplayName: displayName}) } +// NotifyAppDeployStarted records the ACCEPTANCE — the fact `app_deployed` used to assert. It keeps +// the timeline's "the customer asked for this app at 12:31" without claiming the install finished. +// +// NOTE: the hub validates event_type against allowedEventTypes and 400s an unknown one, so this type +// MUST exist there too (hub handler.go) or the event is silently inert. +func (n *Notifier) NotifyAppDeployStarted(stackName, displayName string) { + n.PushEvent("app_deploy_started", "info", + fmt.Sprintf("Alkalmazás telepítése elindult: %s", displayName), + AppDetails{StackName: stackName, DisplayName: displayName}) +} + +// NotifyAppDeployFailed closes the pair. severity=warning, not info: an install the customer started +// and that did not finish is a thing someone should see, and the silent version of this is exactly +// what left a completed-install record for an app that was never installed. +func (n *Notifier) NotifyAppDeployFailed(stackName, displayName, reason string) { + msg := fmt.Sprintf("Alkalmazás telepítése nem sikerült: %s", displayName) + if reason != "" { + msg += " — " + reason + } + n.PushEvent("app_deploy_failed", "warning", msg, + AppDetails{StackName: stackName, DisplayName: displayName}) +} + // AppRunState is one deployed app's running state for the fix-3 start-failure notifier: Down=true // when the app is deployed but its containers are not running. type AppRunState struct { diff --git a/controller/internal/stacks/deploy.go b/controller/internal/stacks/deploy.go index 428ecd7..09bc97f 100644 --- a/controller/internal/stacks/deploy.go +++ b/controller/internal/stacks/deploy.go @@ -401,6 +401,15 @@ func (m *Manager) DeployStack(req DeployRequest) (string, error) { // runComposeDeploy executes docker compose up -d in background. // On success it refreshes status; on failure it reverts the deploy state. +// SetDeployDoneHook registers the callback fired (in the deploy goroutine) when an async deploy +// ENDS — successfully or not. It is the seam R-536 needed: the deploy's outcome is known here and +// nowhere else, and the hub event that asserts an app is installed must hang off the outcome rather +// than off the acceptance. +// +// Same shape as SetMigrationDoneHook, deliberately: the policy (which event, what wording) lives in +// the caller, and this package only reports what happened. +func (m *Manager) SetDeployDoneHook(fn func(name string, ok bool, detail string)) { m.deployDoneHook = fn } + func (m *Manager) runComposeDeploy(name, stackDir string, env map[string]string, appCfg *AppConfig) { start := time.Now() _, composeErr := m.composeExecWithEnv(stackDir, env, "up", "-d") @@ -421,6 +430,15 @@ func (m *Manager) runComposeDeploy(name, stackDir string, env map[string]string, // Save reverted state to disk with encryption (H05 fix) meta := LoadMetadata(stackDir) _ = SaveAppConfig(stackDir, appCfg, m.encKey, SensitiveEnvVars(&meta)) + // R-536: the deploy ended, and it ended badly. Say so — the alternative is the silence that + // let an accept-time „Alkalmazás telepítve" stand as the last word on an app that never ran. + // + // The app.yaml is deliberately NOT deleted here: it is the crash-safe record written with + // Deployed:false, it carries the settings the customer typed, and a redeploy reuses them. The + // state the surfaces read is `not_deployed`, which is the fact that matters. + if m.deployDoneHook != nil { + m.deployDoneHook(name, false, composeErr.Error()) + } return } @@ -472,6 +490,19 @@ func (m *Manager) runComposeDeploy(name, stackDir string, env map[string]string, m.logPostStartStatus(name, stackDir, deployEnv) _ = m.RefreshStatus() + + // R-536: ONLY NOW is „Alkalmazás telepítve" a true sentence — the compose up succeeded, the + // durable record says deployed, the images are recorded and the status has been refreshed. The + // observed state rides along rather than being asserted: a stack that is still `starting` is + // installed, and the detail says which it is instead of the event implying health it has not + // measured. + if m.deployDoneHook != nil { + state := "" + if s, ok := m.GetStack(name); ok { + state = string(s.State) + } + m.deployDoneHook(name, true, state) + } } // UpdateStackConfig updates non-locked fields for a deployed stack. diff --git a/controller/internal/stacks/manager.go b/controller/internal/stacks/manager.go index 19c7636..04d838b 100644 --- a/controller/internal/stacks/manager.go +++ b/controller/internal/stacks/manager.go @@ -205,6 +205,11 @@ type Manager struct { sysDataPath string backupRunning func() bool // mutual exclusion with the backup orchestrator (Change 3) migDoneHook func(*MigrationJob) // fired on successful completion (decommission policy lives in caller) + // deployDoneHook (R-536) fires when an ASYNC deploy reaches its end — ok=true with the observed + // state, or ok=false with the reason. The hub event that says „Alkalmazás telepítve" hangs off + // this and nothing else: it used to be sent beside the 202 that merely accepted the request, so + // an install interrupted five seconds later stayed on the timeline as a completed one. + deployDoneHook func(name string, ok bool, detail string) testSeams *migSeams // nil in production; tests inject fakes // R-51: docker restart policies for DOWN members of mixed stacks. Keyed by // containerName+"|"+state so a transitioned or recreated container re-reads rather than diff --git a/controller/internal/stacks/r536_deploy_done_hook_test.go b/controller/internal/stacks/r536_deploy_done_hook_test.go new file mode 100644 index 0000000..7502501 --- /dev/null +++ b/controller/internal/stacks/r536_deploy_done_hook_test.go @@ -0,0 +1,81 @@ +package stacks + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// withFakeComposeExit is withFakeCompose with a chosen exit code, so the FAILING deploy can be +// driven through the same process boundary as the succeeding one. +func withFakeComposeExit(t *testing.T, m *Manager, code int) { + t.Helper() + if runtime.GOOS != "linux" { + t.Skip("the stub compose binary is a shell script") + } + bin := t.TempDir() + script := "#!/bin/sh\nexit " + string(rune('0'+code)) + "\n" + if err := os.WriteFile(filepath.Join(bin, "docker-compose"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + m.composeCmd = "docker-compose" + m.execFn = func(string, ...string) (string, error) { return "", nil } +} + +// R-536 — „Alkalmazás telepítve" must be said at the END of a deploy, and a deploy that ends badly +// must say THAT rather than nothing. +// +// The defect this pins, measured live on 2026-09-16: the deploy of `mealie` was accepted at +// 12:31:36 CEST and the hub logged „Alkalmazás telepítve: Mealie" in the same second; the controller +// was killed five seconds later, and after the agent restarted it the stack read +// `not_deployed / deployed=false / deploying=false`. Nothing ever corrected the event. +// +// Red-proof: remove the deployDoneHook call from the success path → the "installed" case fails; +// remove it from the failure branch → the "failed" case fails. +func TestDeployDoneHook_FiresAtTheEndAndSaysWhichEndItWas(t *testing.T) { + t.Run("a deploy that succeeds reports installed", func(t *testing.T) { + m, dir := newInstalledManager(t, "services:\n web:\n image: nginx:1.27\n", "deployed: true\nenv: {}\n") + withFakeCompose(t, m) + + var gotName, gotDetail string + var gotOK, fired bool + m.SetDeployDoneHook(func(name string, ok bool, detail string) { + fired, gotName, gotOK, gotDetail = true, name, ok, detail + }) + + m.runComposeDeploy("bookstack", dir, map[string]string{}, &AppConfig{Deployed: true}) + + if !fired { + t.Fatal("the deploy ended and nothing was told about it") + } + if gotName != "bookstack" || !gotOK { + t.Fatalf("a successful deploy must report ok for its own app: name=%q ok=%v detail=%q", gotName, gotOK, gotDetail) + } + }) + + t.Run("a deploy that fails says so", func(t *testing.T) { + m, dir := newInstalledManager(t, "services:\n web:\n image: nginx:1.27\n", "deployed: true\nenv: {}\n") + withFakeComposeExit(t, m, 1) + + var gotOK, fired bool + var gotDetail string + m.SetDeployDoneHook(func(_ string, ok bool, detail string) { + fired, gotOK, gotDetail = true, ok, detail + }) + + m.runComposeDeploy("bookstack", dir, map[string]string{}, &AppConfig{Deployed: true}) + + if !fired { + t.Fatal("a failed deploy must be reported, not be silence — silence is what left a completed-install record for an app that never ran") + } + if gotOK { + t.Fatalf("a failed deploy must not report ok (detail=%q)", gotDetail) + } + // And the durable record must read not-deployed, which is the fact every surface reads. + if cfg := LoadAppConfig(dir); cfg != nil && cfg.Deployed { + t.Fatal("a failed deploy must leave the durable record NOT deployed") + } + }) +} diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index 17d46b7..8ee36b9 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -1174,9 +1174,15 @@ type AppBackupRow struct { StorageLabel string HDDSizeHuman string - // What this app's backup contains (for display) - // e.g., "DB + Konfiguráció + Adatok", "DB + Konfiguráció", "Konfiguráció" - BackupContents string + // What this app's backup contains, PER TIER (R-537). One string for all three tiers was a claim + // no single string can support: a Tier-1 unit holds no copy of the files a class-A app keeps on + // the data drive, while Tier 2 and Tier 3 do. e.g. Tier1Contents "DB + Konfig", + // Tier23Contents "DB + Konfig + Adatok". + Tier1Contents string + Tier23Contents string + // DriveFilesNote is non-empty exactly when this app keeps files on the data drive that a Tier-1 + // unit cannot hold — the one sentence that tells the household where those files ARE protected. + DriveFilesNote string // RestoreHeld (R-379) — this app is deliberately stopped because a database restore failed AND // the rollback failed. Distinct from any backup status: it is about the app's LIVE data, not its @@ -1342,16 +1348,41 @@ func (s *Server) buildAppBackupRows(status *backup.FullBackupStatus) []AppBackup } } - // Build backup contents label - var parts []string + // Build the backup contents labels — ONE PER TIER (R-537). + // + // This used to be a single string rendered on all three tier rows, computed from the APP's + // shape (`HasHDDData || HasVolumeData → "Adatok"`) rather than from what each tier actually + // captures. On a fresh one-drive box that made the Tier-1 row read „DB + Konfig + Adatok" + // over a unit holding a database dump, three volume tars and no copy of the customer's files + // at all — measured 2026-09-16 with five photos that were in no backup while the page said + // they were. + // + // The fact each tier captures is settled in 07-backup-architecture §6.1/§6.2 and is not + // changed here: a Tier-1 unit carries compose + app.yaml + DB dumps + volume tars and has no + // file-copy step; the drive-side file legs of a class-A app are carried by Tier 2 and Tier 3. + // So the label differs by tier, and one string cannot be true for all three. + hasDriveFileLegs := s.backupMgr != nil && s.backupMgr.HasDriveFileLegs(app.StackName) + base := []string{} if hasDB { - parts = append(parts, "DB") + base = append(base, "DB") } - parts = append(parts, "Konfig") - if app.HasHDDData || app.HasVolumeData { - parts = append(parts, "Adatok") + base = append(base, "Konfig") + withData := func(add bool) string { + p := append([]string{}, base...) + if add { + p = append(p, "Adatok") + } + return strings.Join(p, " + ") + } + // Tier 1: „Adatok" only when the app's data really is inside the volumes this unit captured. + // For an app that keeps its files on the drive it is a claim the unit cannot support. + tier1Contents := withData(app.HasVolumeData && !hasDriveFileLegs) + // Tier 2 / Tier 3 carry the file legs, so for them „Adatok" is true either way. + tier23Contents := withData(app.HasVolumeData || hasDriveFileLegs) + driveFilesNote := "" + if hasDriveFileLegs { + driveFilesNote = "Az alkalmazás fájljait a távoli másolat (és a második meghajtó) védi — ez a helyi mentés a beállításokat és az adatbázist tartalmazza." } - contents := strings.Join(parts, " + ") slug := "" if s.stackMgr != nil { @@ -1370,7 +1401,9 @@ func (s *Server) buildAppBackupRows(status *backup.FullBackupStatus) []AppBackup DriveDisconnected: driveDisconnected, StorageLabel: app.StorageLabel, HDDSizeHuman: app.HDDSizeHuman, - BackupContents: contents, + Tier1Contents: tier1Contents, + Tier23Contents: tier23Contents, + DriveFilesNote: driveFilesNote, Tier1DBStatus: tier1DBStatus, } @@ -1513,6 +1546,27 @@ func tier2DestLabel(destPath, systemDataPath string) string { return filepath.Base(strings.TrimSuffix(destPath, "/"+backup.FelhomDataDir)) } +// missingFileLegsRefusal builds the Hungarian sentence shown when a unit restore is refused because +// the unit carries no copy of the app's files (R-538). It names the route that CAN return them, and +// when there is none it says so rather than implying one exists. +// +// The three branches are the three real states, in the order a customer can act on them: the off-site +// copy (a full restore brings files AND database), the second drive (its file half is its own +// action), and nothing. +func (s *Server) missingFileLegsRefusal(ctx context.Context, stackName string) string { + const head = "Ez a mentés nem tartalmazza az alkalmazás fájljait, ezért nem állítjuk vissza az adatbázist föléjük — a fájlok így a helyükön maradnak. " + if s.backupMgr != nil { + rows, _ := s.offsiteRestoreRows(ctx) + if row := resolveOffsiteRestoreApp(rows, stackName); row != nil { + return head + "A fájlok a távoli másolatból állíthatók vissza: Biztonsági mentés → Visszaállítás, „Teljes visszaállítás (fájlok + adatbázis)”." + } + if cov, err := s.backupMgr.Tier2RestoreCoverage(stackName); err == nil && cov.CanRestore() { + return head + "A fájlok a második meghajtó másolatából állíthatók vissza: „Fájlok visszaállítása”." + } + } + return head + "Ezekről a fájlokról jelenleg nincs másolat — kapcsold be a távoli mentést, vagy csatlakoztass egy második meghajtót." +} + func (s *Server) backupRestoreHandler(w http.ResponseWriter, r *http.Request) { _ = r.ParseForm() @@ -1546,6 +1600,24 @@ func (s *Server) backupRestoreHandler(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/backups/restore?flash_error="+url.QueryEscape(msg), http.StatusFound) return } + // R-538: refuse a unit restore that would replay a database over files the unit does not hold — + // BEFORE the op begins, so nothing is stopped and the app's own wastebasket is left intact. The + // customer gets the route that CAN return their files instead of a success message over an app + // listing photos it cannot open. + // + // `accept_missing_files=1` is the explicit, separately-worded second step („csak az adatbázist és + // a beállításokat"). It is deliberately not a sibling of the main button: two controls whose + // difference is "your data comes back" are never siblings (R-48). + acceptMissingFiles := r.FormValue("accept_missing_files") == "1" + if !acceptMissingFiles { + if legs := s.backupMgr.DeclaredDriveFileLegs(stackName); len(legs) > 0 { + msg := s.missingFileLegsRefusal(r.Context(), stackName) + s.logger.Printf("[WARN] [web] restore refused for %s: unit carries no file leg (%d drive path(s))", stackName, len(legs)) + http.Redirect(w, r, "/backups/restore?flash_error="+url.QueryEscape(msg), http.StatusFound) + return + } + } + s.logger.Printf("[WARN] [web] Restore requested (async): stack=%s, snapshot=%s from %s", stackName, snapshotID, r.RemoteAddr) s.backupMgr.BeginRestoreOp("restore", stackName) go func() { diff --git a/controller/internal/web/r537_tier_contents_test.go b/controller/internal/web/r537_tier_contents_test.go new file mode 100644 index 0000000..a474c32 --- /dev/null +++ b/controller/internal/web/r537_tier_contents_test.go @@ -0,0 +1,72 @@ +package web + +import ( + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/appbackup" + "gitea.dooplex.hu/admin/felhom-controller/internal/backup" +) + +// fileLegProvider declares a MANDATORY drive-side bind for one app and nothing for the others — the +// shape of a class-A app (calibre-web, immich, nextcloud, paperless-ngx) against the 45 whose data +// lives entirely in Docker volumes. +type fileLegProvider struct { + blockProvider + withLegs string +} + +func (p *fileLegProvider) GetStackComposePath(string) (string, bool) { return "compose", true } +func (p *fileLegProvider) GetStackClassifiedBinds(n string) ([]backup.ClassifiedBind, bool) { + if n != p.withLegs { + return nil, false + } + return []backup.ClassifiedBind{{ + ComposeBind: appbackup.ComposeBind{Root: appbackup.RootHDD, RelPath: "appdata/" + n}, + Class: appbackup.ClassMandatory, + }}, true +} + +// R-537 — the backup page must describe what each TIER captured, not what the app is shaped like. +// +// The defect this pins, measured live on 2026-09-16 on a fresh box: the Tier-1 row read +// „DB + Konfig + Adatok" for Nextcloud, over a unit that held a database dump, three volume tars and +// no copy of the customer's files at all. The five photos in that folder were in no backup on the +// box, and the page said they were. +// +// Red-proof: put the old single label back (`if app.HasHDDData || app.HasVolumeData → "Adatok"`, +// rendered on all three tier rows) → the Tier-1 assertion fails. +func TestAppBackupRows_Tier1LabelDoesNotClaimFilesItCannotHold(t *testing.T) { + s, _, m := newOffboxWebServer(t) + drive := t.TempDir() + m.SetStackProvider(&fileLegProvider{blockProvider: blockProvider{hdd: drive}, withLegs: "nextcloud"}) + + rows := s.buildAppBackupRows(&backup.FullBackupStatus{AppDataInfo: []backup.AppBackupInfo{ + {StackName: "nextcloud", DisplayName: "Nextcloud", HasHDDData: true, HasVolumeData: true}, + {StackName: "privatebin", DisplayName: "PrivateBin", HasVolumeData: true}, + }}) + + nc := findRow(rows, "nextcloud") + if nc == nil { + t.Fatal("no row for nextcloud") + } + if strings.Contains(nc.Tier1Contents, "Adatok") { + t.Fatalf("the Tier-1 label claims it holds the app's data: %q — the unit has no file leg and the customer's files are on the drive", nc.Tier1Contents) + } + if !strings.Contains(nc.Tier23Contents, "Adatok") { + t.Fatalf("Tier 2/3 DO carry the file legs; their label must say so: %q", nc.Tier23Contents) + } + if nc.DriveFilesNote == "" { + t.Fatal("an app whose files a local backup cannot hold must be told where they ARE protected") + } + + // NEGATIVE CONTROL: an app whose data really is inside the volumes the unit captured keeps its + // „Adatok". Without this, "never say Adatok" would pass and would be a different lie. + pb := findRow(rows, "privatebin") + if pb == nil || !strings.Contains(pb.Tier1Contents, "Adatok") { + t.Fatalf("an app whose data IS in the captured volumes must keep its Adatok label: %+v", pb) + } + if pb.DriveFilesNote != "" { + t.Fatalf("an app with no drive-side files needs no note about them: %q", pb.DriveFilesNote) + } +} diff --git a/controller/internal/web/templates/backups_apps.html b/controller/internal/web/templates/backups_apps.html index 267d9ce..8a96e23 100644 --- a/controller/internal/web/templates/backups_apps.html +++ b/controller/internal/web/templates/backups_apps.html @@ -202,7 +202,8 @@ {{else}}—{{end}} {{end}} - {{.BackupContents}} + {{.Tier1Contents}} + {{if .DriveFilesNote}}{{.DriveFilesNote}}{{end}} {{if and .HasDB (eq .Tier1DBStatus "error")}} DB dump hiba {{end}} @@ -226,7 +227,7 @@ {{else}} Még nincs sikeres másolat {{end}} - {{.BackupContents}} + {{.Tier23Contents}}
Beállítás
@@ -241,7 +242,7 @@ {{else}} Még nincs sikeres másolat {{end}} - {{.BackupContents}} + {{.Tier23Contents}}
Beállítás
@@ -272,7 +273,7 @@ not render as a plain fresh copy. The status line above is about the RUN; this one is about the PACKAGE, and after a preserved leg they differ. */}} {{if .Tier2UnitStaleNotice}}{{.Tier2UnitStaleNotice}}{{end}} - {{.BackupContents}} + {{.Tier23Contents}}
{{if not .Tier2SuccessTracked}}{{if .Tier2LastRun}}