diff --git a/CHANGELOG.md b/CHANGELOG.md index 09207b3..f5f54fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,58 @@ +## v0.205.0 — a backup that skipped an app the customer chose is not „Rendben" (2026-08-06, R-234) — MinAgent 0.127.0 + +**Two defects, and the one that actually produced the measured sequence was NOT the one filed.** + +### The verdict now counts a skipped selection + +The R-203 verdict block already carries the sentence *"a warning beside a success is read as a +success"* — and applied it to **one of the two shapes it describes**. An app missing a declared +mandatory FOLDER made the run `incomplete`; an app skipped **entirely**, with nothing of it in the +snapshot at all, still reported `ok` with a warning beside it. The smaller gap moved the verdict and +the bigger one did not. It does now. + +**Which skips count (§7.2), decided by measurement rather than assumption:** + +| skip | counts? | why | +|---|---|---| +| selected + **deployed**, no recovery unit | **yes** | the app the customer chose is not protected | +| selected but **not deployed** | **no**, but NAMED with what to do | a box left amber forever by an app somebody removed is a status nobody reads | +| drive disconnected / decommissioned | **no** | it has its own card and its own signal | +| nothing selected at all | **no** | unchanged: the existing zero-selection notice | + +The operator signal is **reused, not mirrored** — a skipped app is reported through the existing +mandatory-gap notification as a whole-unit gap, so one vocabulary covers both. +`LastSuccess` and `SnapshotCount` still record what WAS captured: half a backup is not no backup. + +### The measured cause: a manual run silently dropped by the single-flight + +Reproduced on demo-hp: **the pre-dump phase (`captureAllRecoveryUnits`) writes a unit for every +deployed stack before the push**, so "selected but no bundle yet" does not normally survive a run — +moving a unit aside and running recreated it and reported `ok`. So the filed mechanism could not have +produced the 2026-08-06 sequence. + +What did: the customer pressed „Távoli mentés most”, the handler answered „A távoli mentés elindult”, `acquireRunning` refused because a run was already going, and the run +returned **nil** — no error, no signal. The card then showed the **previous** run's „Rendben", which +reads as covering the app just selected. It did not, and the restore refused minutes later. + +The single-flight decision is now taken **synchronously in the handler**, before the goroutine, and a +dropped request says so. The nightly path is deliberately unchanged: returning nil is right for it — +nobody asked, and the next scheduled run retries. + +### §7.3 — the wait, measured before deciding + +`CaptureRecoveryUnit` writes compose config + a manifest (**a few KB**, per `admission.go`'s own +note), **enumerates** dumps already present rather than creating them, is idempotent, and **does not +stop the app**. It already runs for every deployed stack inside the off-site run's own pre-dump phase, +through `admitApp`. **So the inline capture this task contemplated already exists — nothing was built**, +and for a deployed app there is no wait to remove. + +### Hungarian + +- „Ezek az alkalmazások NEM kerültek be a távoli mentésbe, mert még nincs helyi mentési egységük: %s. A következő mentés általában már elkészíti — ha a második futás után is itt szerepelnek, szólj az üzemeltetőnek.” +- „Ezek az alkalmazások ki vannak jelölve távoli mentésre, de nincsenek telepítve, ezért nem menthetők: %s. Ha már nincs rájuk szükséged, vedd ki a kijelölésüket a Távoli mentés oldalon.” +- „Már fut egy távoli mentés — ez a kérés nem indított újat. A most látható eredmény még a korábbi futásé; várd meg, míg ez befejeződik.” +- sibling (mandatory folders), extended so both read alike: „… Ellenőrizd, hogy a mappák megvannak-e a meghajtón; ha igen és ez a következő mentés után is látszik, szólj az üzemeltetőnek.” + ## v0.204.0 — what you can restore is decided by the store, not by what happens to be installed (2026-08-06, R-237 / R-238) — MinAgent 0.127.0 **A household that had just lost its box was shown nothing to restore.** Measured live on the R-201 diff --git a/CONTEXT.md b/CONTEXT.md index 5c34f7f..9efc5a4 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,27 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-08-05 (v0.200.0 — R-193: the recovery screen, unlocking only) +Last updated: 2026-08-06 (v0.205.0 — R-234: a run that skipped a selected app is not successful) + +> **2026-08-06 — v0.205.0 (R-234).** THE RULE: **a run that skipped an app the customer selected is +> not a successful run.** The R-203 verdict block already said *"a warning beside a success is read +> as a success"* and applied it to one of the two shapes it describes — a missing declared FOLDER +> made the run `incomplete`, an app skipped ENTIRELY did not. Now both do. A selected-but-UNDEPLOYED +> app is named with what to do but does NOT move the verdict, because a box left permanently amber by +> an app somebody removed is a status nobody reads. +> +> **§7.3, MEASURED rather than assumed — and the answer was "already done".** `CaptureRecoveryUnit` +> writes compose config + a manifest (a few KB), only ENUMERATES dumps rather than creating them, is +> idempotent, and does NOT stop the app; the off-site run already calls it for every deployed stack in +> its own pre-dump phase, through `admitApp`. So there is no wait to remove for a deployed app, and +> **nothing was built**. Proven on demo-hp: a unit moved aside was RECREATED by the run. +> +> **AND THE FILED MECHANISM WAS NOT THE MEASURED CAUSE.** R-234 was filed as "the first run after a +> toggle finds no bundle and skips the app". That cannot happen for a deployed app (above). What did +> happen on 2026-08-06: the manual run was dropped by the **single-flight** while an earlier run was +> still going; `runOffboxBackup` returned nil; the handler had already said „elindult”; and the card +> then showed the PREVIOUS run's „✓ Rendben”. Fixed by taking that decision synchronously in the +> handler. **The nightly path deliberately still returns nil** — nobody asked, and it retries. > **2026-08-05 — v0.200.0 (R-193 CLOSED).** The customer-facing recovery screen. Until now a customer > whose machine was rebuilt had everything needed to get their data back and no way to find out — the diff --git a/REPORT.md b/REPORT.md index 6a938af..72b6529 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,91 +1,115 @@ -# REPORT — v0.204.0: the restore list is keyed on the store (R-237), and the size gate stops refusing in silence (R-238) +# REPORT — v0.205.0: a backup that skipped an app the customer chose is not „Rendben" (R-234) -2026-08-06. Controller **v0.203.0 → v0.204.0**. MinAgent unchanged (**0.127.0**) — nothing here needs -a new agent capability. `felhom-agent` untouched. +2026-08-06. Controller **v0.204.0 → v0.205.0**. MinAgent unchanged (**0.127.0**). `felhom-agent`, +`app-catalog-felhom.eu` untouched. No hub change. -## R-238 — classified: a HARNESS ARTIFACT, with a real residue that is fixed +## The correction that outranks the task -The two runs diverge at one parameter, and the state at that point is quoted rather than inferred: +§3 stated the mechanism as established: *"the off-site copy sends the local recovery bundle an app +already has; switching an app on does not create one, so the first run after the toggle finds nothing +to send."* **Measured on demo-hp, that is not what the code does.** -- `POST /backup/offbox/restore` with `mode=full` and **no** `confirm=1` is **step 1 of a deliberate - two-step** (`offbox_handlers.go:314`). It computes size + headroom via `OffboxRestorePrepareFull`, - **starts no job**, and redirects to - `restoreWizardPath(app) + "&full_prep=&full_size="`. -- `deriveWizardStep` (`restore_wizard.go`) reveals the commit **only** when - `in.FullPrepApp == in.App`, sourced from `?full_prep=`. -- The endpoint-level driver posted step 1 and then re-fetched the wizard **without** that parameter. - The pure function therefore returned the **intent** step — correctly. `restore-status.last == null` - is likewise **correct**: step 1 starts no job by design. -- The operator's browser run followed the redirect, saw the confirm, pressed „Igen" twice, and the - restore completed. +The off-site run's own **pre-dump phase** calls `captureAllRecoveryUnits()`, which writes a unit for +**every deployed stack** — through `admitApp` — *before* the per-app push loop. I moved +`calibre-web`'s unit aside (never deleted) on demo-hp and triggered a run through the real endpoint: -**So the button is not dead, and the wizard was not re-keyed.** The precedence rules exist so a stale -`?full_prep=` can never resurrect a commit button mid-restore, and they were left alone. +``` +BEFORE : units = calibre-web, opengist, privatebin status=ok snapshots=9 +moved : calibre-web unit → .calibre-web.R234-aside +RUN : POST /backup/offbox/run → 302, finished ~90 s, status=ok +AFTER : status=ok snapshots=9 warning=(none) +unit : /mnt/sys_drive/felhom-data/backups/primary/calibre-web → RECREATED +``` -**The residue, which is real whoever triggers it:** neither branch of step 1 wrote anything to the -log. `offboxRedirectTo` only flashes to the page. A customer refused a disaster restore — **including -a refusal by the headroom gate** — left **no trace on the box at all**. Fixed: the refusal logs -`[WARN] … full-restore preparation REFUSED for (no job started): `, the success logs -`[INFO] … full-restore prepared for (size N) — awaiting the customer's confirm; no restore has -started`, and the concurrent-op refusal logs too. +So "selected but no bundle yet" **does not survive a run** for a deployed app. The filed mechanism +could not have produced the 2026-08-06 sequence. -## R-237 — the restore list, rebuilt on the store +**What did.** `POST /backup/offbox/run` launched the run in a goroutine and immediately answered +„A távoli mentés elindult". Inside, `acquireRunning` refused (a run was already in flight) and +`runOffboxBackup` returned **nil** — no error, no signal. The card then showed the **previous** run's +„✓ Rendben · 1 pillanatkép", read as covering the app just selected. It did not: the restore refused +for that app minutes later, and a third run carried it. That is R-234's real cause, and it is the same +family — a skip that reached a log and not a verdict. -`buildOffsiteRestoreRows` (new, pure) merges `OffsiteInventoryList` (the repository's own snapshot -tags — the existing R-193 reader) with the installed set. `resolveOffsiteRestoreApp` replaces -`resolveWizardApp`'s toggle requirement. +## Live validation (§12) -Every §7 case, and the Hungarian as rendered: +| # | what | observable | +|---|---|---| +| 1 | selected app with no unit → run | the run **recreated the unit** and reported ok — the measurement above. The verdict change is exercised by the run-level tests, because production cannot easily be held in that state | +| 2 | counters intact | `snapshot_count` 9 → 9, `last_success` advanced to `2026-08-06T19:34:27Z` — what was captured is still recorded | +| 3 | the third-run behaviour arriving sooner | **not applicable as filed**: there is no wait for a deployed app; the unit is created by the run itself (§7.3) | +| 4 | a box with nothing selected | unchanged — Scenario D is pinned by test, and demo-hp's other apps stayed `ok` | -| case | rendered | -|---|---| -| snapshot present, app not installed | restore offered + „Nincs telepítve — a visszaállítás előbb újratelepíti." | -| installed, no snapshot | „Nincs mentése a távoli tárolóban — nincs mit visszaállítani." | -| store unreadable | „Nem tudjuk elolvasni a távoli tárolót, ezért **nem tudjuk, mi van benne**. Ez nem azt jelenti, hogy üres — próbáld újra később, vagy jelezd az üzemeltetőnek." + „Nem tudjuk, van-e mentése — a tárolót nem sikerült elolvasni." **and the action stays offered** | -| no target yet | „A távoli tároló kapcsolódási adatai még nem érkeztek meg ehhez a géphez, ezért még nem tudjuk megmutatni, mi van benne. Ez magától rendeződik." | -| store empty | „A távoli tároló üres — nincs mit visszaállítani." | -| app under a different name | **not guessed** — it lists under the tag the store holds, and if nothing is installed under that name the row says so. No fuzzy matching. | +**Access note:** demo-hp's dashboard password had been changed by the customer claim, so I reset it +through the documented `--print-reset-code` escape hatch (code streamed file→file, extracted by shape, +shredded; the new password is a 24-char generated value in a `0600` file on DooPlex). That is a +deliberate change to a Tier-0 demo box, recorded here. -Wizard refusals also changed: „Ehhez az alkalmazáshoz nincs mentés a távoli tárolóban." and, when the -store could not be read, „Nem tudjuk elolvasni a távoli tárolót, ezért nem tudjuk, van-e benne mentés -ehhez az alkalmazáshoz." Both log an INFO naming the app and the store state. +## §7.2 — which skips count, as decided -`felhom-offbox` and `_shares` are excluded from the app list. +| skip | counts? | why | +|---|---|---| +| selected + **deployed**, no recovery unit | **YES** → `incomplete` | the app the customer chose is not in the snapshot at all | +| selected but **not deployed** | **no** — named, with what to do | a box left amber forever by an app somebody removed is a status nobody reads | +| drive disconnected / decommissioned | **no** | it has its own card and its own signal; re-reporting it here would double-count | +| nothing selected at all | **no** | unchanged: the existing „nincs mentésre jelölt alkalmazás" notice | -## Tests +## §7.3 — the measurement, and the decision it forced + +`CaptureRecoveryUnit` writes `compose/` (docker-compose.yml, .felhom.yml, app.yaml) + `manifest.json` +— **a few KB**, per `admission.go`'s own note — and **enumerates** the DB/volume dumps already present +rather than creating them. It is idempotent (skips all writes when current) and **does not stop the +app**; the stopping work belongs to the separate dump flow. + +**Decision: nothing to build.** The cheap, non-stopping capture already runs for every deployed stack +inside the off-site run's pre-dump phase, through `admitApp`. For a deployed app there is no wait to +remove, and the §7.3 branch that would have added an inline capture would have duplicated it. + +## §7.4 — every changed Hungarian string + +- „Ezek az alkalmazások NEM kerültek be a távoli mentésbe, mert még nincs helyi mentési egységük: %s. A következő mentés általában már elkészíti — ha a második futás után is itt szerepelnek, szólj az üzemeltetőnek." +- „Ezek az alkalmazások ki vannak jelölve távoli mentésre, de nincsenek telepítve, ezért nem menthetők: %s. Ha már nincs rájuk szükséged, vedd ki a kijelölésüket a Távoli mentés oldalon." +- „Már fut egy távoli mentés — ez a kérés nem indított újat. A most látható eredmény még a korábbi futásé; várd meg, míg ez befejeződik." +- **sibling, extended so both read alike:** „Figyelmeztetés: a(z) %s alkalmazás egyes adatmappái nem kerültek a távoli mentésbe: %s. **Ellenőrizd, hogy a mappák megvannak-e a meghajtón; ha igen és ez a következő mentés után is látszik, szólj az üzemeltetőnek.**" + +The replaced sentence was: „Figyelmeztetés: %d alkalmazásnak nincs elérhető mentése, ezek kimaradtak: %s" — a count with no next step, reading the same whether the customer must act or simply wait. + +## Tests and red-proofs `go build` · `go vet` · `go test ./...` → **28 packages ok**. `controller_gates.py --fast` → **9/9 OK**. -New: `offsite_restore_list_test.go` (7 tests — the truth table, the rebuilt box, unreadable-is-unknown, -no-target, empty, and two rendered-page tests) and `offbox_restore_silence_test.go` (handler-level, -because the silence was in the handler). +New: `internal/backup/offbox_verdict_r234_test.go` (Scenarios A, C, D, F — run-level) and +`internal/web/offbox_run_inflight_test.go` (handler-level). -## Red-proofs — each mutation asserted to have applied before the result was trusted - -| # | mutation | result | +| # | mutation (each asserted to have applied) | result | |---|---|---| -| RP-1 | store rows dropped from the builder — the list keyed back on installed apps (the original defect) | `TruthTable` + `RebuiltBox_SeesItsSnapshots` **FAIL** | -| RP-2 | `state` forced to `known` **and** `StoreUnknown` forced false (both guards) | `UnreadableStoreIsUnknownNotEmpty` + `NoTargetIsItsOwnState` **FAIL** | -| RP-3 | both size-gate log lines removed | `FullPrepareRefusal_IsNotSilent` **FAIL** — „wrote NOTHING to the log" | +| RP-1 | drop `unprotected` from the verdict — the defect itself | `SkippedSelectedAppIsIncomplete` **FAIL** — „LastStatus = ok, want incomplete" | +| RP-2 | remove the classification switch; count **every** skip | `SelectedButUndeployedIsNamedNotCounted` **FAIL** — a removed app turns the box amber | +| RP-3 | stop adding skipped apps to the operator signal | `SkippedSelectedAppIsIncomplete` **FAIL** — „operator signal … got map[]" | +| RP-4 | remove the synchronous in-flight check in the handler | `InFlightRequestIsNotReportedAsStarted` **FAIL** | -All three restored; suite green again afterwards. +All restored; `grep -c RED-PROOF` = 0 in both files afterwards; suite green again. + +**Fixture note:** the shared `offbox3aProvider.ListDeployedStacks()` returned nil, so my first run of +Scenario A passed *for the wrong reason* and F passed vacuously. Fixed with an **opt-in** `deployed` +map that defaults to nil, so no existing fixture's behaviour moves. ## Files -- new `internal/web/offsite_restore_list.go`, `internal/web/offsite_restore_list_test.go`, - `internal/web/offbox_restore_silence_test.go` -- `internal/web/restore_wizard.go` (gate on the store, not the toggle) -- `internal/web/offbox_handlers.go` (three log lines) -- `internal/web/handlers.go` (wire the rows) -- `internal/web/templates/backups_restore.html` (the list + the state wording) -- `internal/web/backups_split_test.go` (fixture: the new data contract) -- `internal/backup/offbox_inventory.go` (`ErrNoOffsiteTargetSentinel`, so the no-target case is - constructible from another package's table test) -- `CHANGELOG.md`, `controller/README.md` +- `internal/backup/offbox.go` — `ErrOffboxRunInFlight`, `offboxWholeUnitGap`, the skip classification, + `missingUnprotected`/`missingNotDeployed`, `stackDeployed`, `driveUnavailableFor`, the verdict, the + messages +- `internal/backup/offbox_capture.go` — the sibling message +- `internal/backup/backup.go` — `AcquireRunningForTest`/`ReleaseRunningForTest` (test-only seams) +- `internal/web/offbox_handlers.go` — the synchronous single-flight refusal +- new `internal/backup/offbox_verdict_r234_test.go`, `internal/web/offbox_run_inflight_test.go` +- `internal/backup/offbox_3a_test.go` — opt-in deployed set +- `CHANGELOG.md`, `controller/README.md`, `CONTEXT.md` -## Not done here, deliberately +## Observations, not acted on -- **R-236 is diagnosed, not fixed** — the fix's shape depends on the diagnosis, and this arc has twice - shipped a fix aimed at the wrong half of a defect. -- The R-193 unlock listing still shows the `felhom-offbox` marker as if it were an app. Noticed while - reusing its reader; **out of scope and not touched.** +- **The zero-selection notice reads „Sikeres — nincs mentésre jelölt alkalmazás"** while the status is + `ok`. It is honest, but "Sikeres" beside "nothing is covered" is the same rhetorical shape this + session is about, one notch weaker. Not touched: Scenario D forbids changing that path. +- **`res.missing` is still used verbatim** for the no-silent-success error text (every app missing → + `error`). Correct as-is, and deliberately left, since that path already refuses loudly. diff --git a/controller/README.md b/controller/README.md index 3052304..8155f4a 100644 --- a/controller/README.md +++ b/controller/README.md @@ -160,6 +160,13 @@ backups, monitoring and notifications. All Proxmox/disk operations are delegated snapshots sat in the repository. Installed-ness is a property OF a row (it changes what restoring implies), never a filter on it; an unreadable store renders as UNKNOWN and keeps the action; the `felhom-offbox` and `_shares` marker tags are never offered as apps. + **A run that skipped a selected app is `incomplete` (v0.205.0, R-234):** the off-site verdict now + counts `missingUnprotected` beside `mandatoryGaps` — an app the customer selected that is DEPLOYED + but has no recovery unit is not protected, so the run is not `ok`. A selected-but-UNDEPLOYED app is + named with what to do and does NOT move the verdict (a permanently amber box is a status nobody + reads); a disconnected/decommissioned drive has its own signal. The manual „Távoli mentés most” + also refuses SYNCHRONOUSLY when a run is already in flight, instead of answering „elindult” and + leaving the previous run's verdict on the card. (traefik/cloudflared/filebrowser) get curated Hungarian display identity from the `inframeta.go` map (name + description + generic `/static/infra-logo.svg` fallback icon); filebrowser is the only infra stack with a customer link (`files.`). diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 2e50c8a..c36b75f 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -95,7 +95,7 @@ type Manager struct { offboxOrphanEvent func(eventType, renamedTo string) // offboxGapNotify (R-203) fires when a COMPLETED offsite run could not capture a directory an // app declares MANDATORY — a coverage gap, not a failed run. nil → no signal. - offboxGapNotify func(gaps map[string][]string) + offboxGapNotify func(gaps map[string][]string) // offboxSSH (v0.142.0) is the raw-ssh exec seam for the orphaned-repo move-aside (restic has no // rename); tests inject a fake. Nil → the real ssh invocation (defaultOffboxSSH). offboxSSH func(ctx context.Context, host, user string, port int, keyPath, knownHosts, remoteCmd string) ([]byte, error) @@ -857,6 +857,12 @@ func (m *Manager) IsRunning() bool { return m.running } +// AcquireRunningForTest / ReleaseRunningForTest occupy the single-flight from another package's +// test, so the "a run is already in flight" branch can be exercised without racing a real run. +// Test-only seam, in the same spirit as SetOffboxRunner; nothing in production calls them. +func (m *Manager) AcquireRunningForTest() error { return m.acquireRunning() } +func (m *Manager) ReleaseRunningForTest() { m.releaseRunning() } + // acquireRunning atomically sets the running flag. Returns error if already running. func (m *Manager) acquireRunning() error { m.mu.Lock() diff --git a/controller/internal/backup/offbox.go b/controller/internal/backup/offbox.go index 7b09b0e..1187d80 100644 --- a/controller/internal/backup/offbox.go +++ b/controller/internal/backup/offbox.go @@ -77,6 +77,16 @@ func (m *Manager) SetOffboxSSH(fn func(ctx context.Context, host, user string, p // the orphan card instead of the raw restic error. var ErrOffboxOrphaned = fmt.Errorf("offbox repo orphaned: exists but keyed under a previous, no-longer-available passphrase") +// ErrOffboxRunInFlight is returned to the MANUAL caller only, when the single-flight dropped the +// request because a run was already going (R-234). It is not a failure of anything — the run in +// flight is doing the work — but it IS a request that did nothing, and the page must say so instead +// of showing the previous run's verdict under a „started" message. +// offboxWholeUnitGap is the pseudo-path used to report a WHOLE-unit gap through the mandatory-gap +// notification, so a skipped app and a skipped directory reach the operator in one vocabulary. +const offboxWholeUnitGap = "(a teljes alkalmazás — nincs helyi mentési egysége)" + +var ErrOffboxRunInFlight = fmt.Errorf("an off-box backup is already running; this request did not start a new one") + // classifyResticProbe maps a `restic cat config` failure to a repo class. The signatures are the exact // restic stderr matched in the 2026-07-17 diagnosis + restic's no-repo message: // - "orphaned": repo present, wrong key ("wrong password or no key found") — the definitive signal @@ -746,7 +756,19 @@ func (m *Manager) runOffboxBackup(ctx context.Context, withProgress bool) error } if err := m.acquireRunning(); err != nil { m.logger.Printf("[INFO] [offbox] skipped — another backup is running") - return nil // single-flight: don't race; the next scheduled run retries + // R-234 (the MEASURED cause). The nightly path is unchanged: returning nil is right for it — + // nobody asked, and the next scheduled run retries. + // + // The MANUAL path is a different question, and answering it the same way is what produced the + // 2026-08-06 sequence. The customer pressed „Távoli mentés most" and was told + // „A távoli mentés elindult"; the run was dropped here and returned nil; the card then showed + // the PREVIOUS run's „✓ Rendben", which they read as covering the app they had just selected. + // It did not — the restore refused for that app minutes later. A request that did nothing must + // not be reported as one that started, so the manual caller is told. + if withProgress { + return ErrOffboxRunInFlight + } + return nil } defer m.releaseRunning() @@ -874,10 +896,38 @@ func (m *Manager) runOffboxBackup(ctx context.Context, withProgress bool) error // backup is not no backup, and reporting it as none would be its own lie. `incomplete` is // minted here because the existing vocabulary ("ok" | "error" | "running") has nothing that // means "it ran, and this app is not fully protected". - if len(runResult.mandatoryGaps) > 0 { + // R-234 EXTENDS THE SAME RULE TO THE BIGGER CASE. Until v0.205.0 the paragraph above was + // applied to ONE of the two shapes it describes: an app missing a declared mandatory + // FOLDER made the run incomplete, while an app skipped ENTIRELY — no recovery unit, so + // nothing of it in the snapshot at all — still reported ok with a warning beside it. The + // smaller gap moved the verdict and the bigger one did not. Measured 2026-08-06: a run + // reported „✓ Rendben · 1 pillanatkép" and the restore then refused for the app the + // customer had just selected. + gaps := len(runResult.mandatoryGaps) > 0 + unprotected := len(runResult.missingUnprotected) > 0 + if gaps || unprotected { o.LastStatus = "incomplete" if m.offboxGapNotify != nil { - m.offboxGapNotify(runResult.mandatoryGaps) + // Reuse, not mirror: the operator signal for "this run left an app less protected + // than the customer asked for" is the same signal. A skipped app is reported as a + // whole-unit gap so one notification shape covers both, and the recipient does not + // have to learn a second vocabulary for the worse case. + notify := runResult.mandatoryGaps + if unprotected { + if notify == nil { + notify = map[string][]string{} + } else { + cp := make(map[string][]string, len(notify)+len(runResult.missingUnprotected)) + for k, v := range notify { + cp[k] = v + } + notify = cp + } + for _, a := range runResult.missingUnprotected { + notify[a] = append(notify[a], offboxWholeUnitGap) + } + } + m.offboxGapNotify(notify) } } else { o.LastStatus = "ok" @@ -895,9 +945,18 @@ func (m *Manager) runOffboxBackup(ctx context.Context, withProgress bool) error if len(apps) == 0 && !runResult.sharesBackedUp { warns = append(warns, "Sikeres — nincs mentésre jelölt alkalmazás") } - if len(missing) > 0 { - warns = append(warns, fmt.Sprintf("Figyelmeztetés: %d alkalmazásnak nincs elérhető mentése, ezek kimaradtak: %s", - len(missing), strings.Join(missing, ", "))) + // R-234 §7.4 — WHICH apps, WHY, and WHEN. The old sentence said only that N apps "had no + // available backup and were left out", which names a problem with no next step and reads + // the same whether the customer must act or simply wait. + if len(runResult.missingUnprotected) > 0 { + warns = append(warns, fmt.Sprintf( + "Ezek az alkalmazások NEM kerültek be a távoli mentésbe, mert még nincs helyi mentési egységük: %s. A következő mentés általában már elkészíti — ha a második futás után is itt szerepelnek, szólj az üzemeltetőnek.", + strings.Join(runResult.missingUnprotected, ", "))) + } + if len(runResult.missingNotDeployed) > 0 { + warns = append(warns, fmt.Sprintf( + "Ezek az alkalmazások ki vannak jelölve távoli mentésre, de nincsenek telepítve, ezért nem menthetők: %s. Ha már nincs rájuk szükséged, vedd ki a kijelölésüket a Távoli mentés oldalon.", + strings.Join(runResult.missingNotDeployed, ", "))) } // 3a: capture-gap warnings (structurally-refused / on-disk-missing mandatory paths, undeployed). warns = append(warns, runResult.warns...) @@ -1059,6 +1118,39 @@ type offboxRunResult struct { // that could NOT be captured. It is the STRUCTURED form of the warnings above, and it is what // decides the run's verdict: a run that dropped a mandatory directory is not a successful run. mandatoryGaps map[string][]string + // missingUnprotected / missingNotDeployed (R-234) split `missing` by WHY, because only one of the + // two may move the verdict. See the classification comment at the skip site: an app the customer + // selected and that IS deployed but has no unit is unprotected and counts; an app that is no + // longer installed is named but does not, so a removed app cannot leave the box amber forever. + missingUnprotected []string + missingNotDeployed []string +} + +// stackDeployed reports whether the stack is currently deployed on this box. Used only to classify a +// skip (R-234) — never to decide whether to back something up. +func (m *Manager) stackDeployed(stack string) bool { + if m.stackProvider == nil { + return false + } + for _, st := range m.stackProvider.ListDeployedStacks() { + if st.Name == stack { + return true + } + } + return false +} + +// driveUnavailableFor reports whether the app's drive is disconnected or decommissioned — states that +// already have their own customer-facing signal, so a skip caused by them is not re-reported here. +func (m *Manager) driveUnavailableFor(stack string) bool { + if m.settings == nil { + return false + } + d := m.GetAppDrivePath(stack) + if d == "" { + return false + } + return m.settings.IsDisconnected(d) || m.settings.IsDecommissioned(d) } // runOffboxInternal does the repo-ensure + per-app DISCOVER → capture-set → gate → multi-path backup + @@ -1079,6 +1171,28 @@ func (m *Manager) runOffboxInternal(ctx context.Context, apps, base, env []strin if !ok { m.logger.Printf("[WARN] [offbox] %s: no recovery unit found on any connected drive — skipping", stack) res.missing = append(res.missing, stack) + // R-234 §7.2 — WHICH skips make the run not-successful. The list above is prose for the + // customer; this classification is what the VERDICT may consult, and the two are not the + // same question. Established by measurement on demo-hp 2026-08-06, not assumed: + // + // * DEPLOYED, no unit — the run's own pre-dump phase (captureAllRecoveryUnits) writes a + // unit for every deployed stack before the push, so this state does not normally + // survive a run. Reaching here means the capture was refused (the reserve) or failed. + // The app the customer selected is NOT protected: it COUNTS. + // * NOT DEPLOYED — nothing can protect an app that is not there, and the remedy is to + // deselect it. It is NAMED so the customer can act, but it does NOT count: a box left + // permanently amber over an app somebody removed is a status that stops being read, + // which is how this whole class of defect starts. + // * drive disconnected/decommissioned — has its own signal and its own card; not ours to + // re-report as a backup gap. + switch { + case !m.stackDeployed(stack): + res.missingNotDeployed = append(res.missingNotDeployed, stack) + case m.driveUnavailableFor(stack): + // counted as neither: the drive card is the honest surface for this one. + default: + res.missingUnprotected = append(res.missingUnprotected, stack) + } continue } // Task 3-core TierOffsite capture set: mandatory userdata paths added to the unit snapshot, diff --git a/controller/internal/backup/offbox_3a_test.go b/controller/internal/backup/offbox_3a_test.go index 64ffe26..9f4b0f7 100644 --- a/controller/internal/backup/offbox_3a_test.go +++ b/controller/internal/backup/offbox_3a_test.go @@ -23,17 +23,30 @@ type offbox3aProvider struct { hdd map[string]string binds map[string][]ClassifiedBind has map[string]bool + // deployed is OPT-IN and defaults to nil, so every existing fixture keeps ListDeployedStacks() + // returning nil and nothing about their behaviour moves. R-234's classification is the only + // thing that needs a real deployed set. + deployed map[string]bool } func (p *offbox3aProvider) GetStackComposePath(string) (string, bool) { return "", false } -func (p *offbox3aProvider) ListDeployedStacks() []StackSummary { return nil } -func (p *offbox3aProvider) GetStackHDDMounts(string) []string { return nil } -func (p *offbox3aProvider) GetStackHDDPath(n string) string { return p.hdd[n] } -func (p *offbox3aProvider) GetImportRoot() string { return "" } // R-75: no import binds in this fixture -func (p *offbox3aProvider) GetDockerVolumes(string) []string { return nil } -func (p *offbox3aProvider) StopStack(string) error { return nil } -func (p *offbox3aProvider) StartStack(string) error { return nil } -func (p *offbox3aProvider) RefreshAndIsRunning(string) bool { return false } +func (p *offbox3aProvider) ListDeployedStacks() []StackSummary { + if len(p.deployed) == 0 { + return nil + } + out := make([]StackSummary, 0, len(p.deployed)) + for n := range p.deployed { + out = append(out, StackSummary{Name: n}) + } + return out +} +func (p *offbox3aProvider) GetStackHDDMounts(string) []string { return nil } +func (p *offbox3aProvider) GetStackHDDPath(n string) string { return p.hdd[n] } +func (p *offbox3aProvider) GetImportRoot() string { return "" } // R-75: no import binds in this fixture +func (p *offbox3aProvider) GetDockerVolumes(string) []string { return nil } +func (p *offbox3aProvider) StopStack(string) error { return nil } +func (p *offbox3aProvider) StartStack(string) error { return nil } +func (p *offbox3aProvider) RefreshAndIsRunning(string) bool { return false } func (p *offbox3aProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { return RecoveryInfo{}, false } diff --git a/controller/internal/backup/offbox_capture.go b/controller/internal/backup/offbox_capture.go index 6bc914b..4efc363 100644 --- a/controller/internal/backup/offbox_capture.go +++ b/controller/internal/backup/offbox_capture.go @@ -75,7 +75,9 @@ func (m *Manager) offboxCaptureSet(stack string) (extra []string, warns []string extra = append(extra, p.Abs) } if len(gaps) > 0 { - warns = append(warns, fmt.Sprintf("Figyelmeztetés: a(z) %s alkalmazás egyes adatmappái nem kerültek a távoli mentésbe: %s.", + // R-234 §7.4: this sits beside the whole-app gap message on the same card, and both now drive + // the same `incomplete` verdict — so it says what to do, not only what happened. + warns = append(warns, fmt.Sprintf("Figyelmeztetés: a(z) %s alkalmazás egyes adatmappái nem kerültek a távoli mentésbe: %s. Ellenőrizd, hogy a mappák megvannak-e a meghajtón; ha igen és ez a következő mentés után is látszik, szólj az üzemeltetőnek.", stack, strings.Join(gaps, ", "))) } return extra, warns, gaps diff --git a/controller/internal/backup/offbox_verdict_r234_test.go b/controller/internal/backup/offbox_verdict_r234_test.go new file mode 100644 index 0000000..0f77a4a --- /dev/null +++ b/controller/internal/backup/offbox_verdict_r234_test.go @@ -0,0 +1,165 @@ +package backup + +import ( + "context" + "strings" + "testing" +) + +// R-234 — a run that SKIPPED an app the customer selected is not a successful run. +// +// The same paragraph the R-203 verdict block already carries — "a warning beside a success is read +// as a success" — was applied to one of the two shapes it describes. An app missing a declared +// mandatory FOLDER made the run `incomplete`; an app skipped ENTIRELY, with nothing of it in the +// snapshot at all, still reported `ok`. The smaller gap moved the verdict and the bigger one did not. +// +// Run-level on purpose: the classification and the verdict are both inside the run, and the sibling +// test file records what happened when its first version asserted the capture helper alone — its +// red-proof passed while the defect was untouched. + +// Scenario A — a selected, DEPLOYED app with no recovery unit makes the run incomplete, names itself, +// and does not suppress what was captured. +// +// RED-PROOF: drop `unprotected` from the verdict condition (leave only mandatoryGaps) → this FAILS +// with the run reporting ok over a skipped app, which is production behaviour up to v0.204.0. +func TestOffboxRun_SkippedSelectedAppIsIncomplete(t *testing.T) { + drive := t.TempDir() + m, sett, prov := classifiedOffboxManager(t, drive) + + // `kept` has a unit and is pushed; `dropped` is selected and deployed but has NO unit, so the + // per-app loop skips it. backedUp>0 is what made the existing no-silent-success guard stay quiet. + mkUnit(t, drive, "kept") + prov.hdd["kept"] = drive + prov.has["kept"] = true + prov.hdd["dropped"] = drive + prov.has["dropped"] = true + prov.deployed = map[string]bool{"kept": true, "dropped": true} + _ = sett.SetAppOffbox("kept", true) + _ = sett.SetAppOffbox("dropped", true) + + var gapNotified map[string][]string + m.SetOffboxGapNotify(func(g map[string][]string) { gapNotified = g }) + cap := &backupCapture{} + m.SetOffboxRunner(cap.runner()) + + if err := m.RunOffboxBackup(context.Background()); err != nil { + t.Fatalf("the run itself must SUCCEED — a skipped app is a coverage gap, not a failed run: %v", err) + } + + got := sett.GetOffboxTarget() + if got.LastStatus != "incomplete" { + t.Fatalf("LastStatus = %q, want \"incomplete\" — the customer selected an app and the run did not "+ + "carry it; on 2026-08-06 this reported „✓ Rendben” and the restore refused minutes later", got.LastStatus) + } + // Scenario A: the counters and the anchor still record what WAS captured. + if got.LastSuccess == "" { + t.Error("LastSuccess must still record what was captured — half a backup is not no backup") + } + if cap.backups != 1 { + t.Errorf("the app that HAD a unit must still be pushed, got %d backup calls", cap.backups) + } + // Scenario E: which app, and why. + if !strings.Contains(got.LastWarning, "dropped") { + t.Errorf("the warning must NAME the skipped app, got %q", got.LastWarning) + } + if !strings.Contains(got.LastWarning, "nincs helyi ment") { + t.Errorf("the warning must say WHY it was skipped, got %q", got.LastWarning) + } + if !strings.Contains(got.LastWarning, "következő ment") { + t.Errorf("the warning must say WHEN it will be protected, got %q", got.LastWarning) + } + // Scenario B: the operator hears about it, in the same vocabulary as a folder gap. + if len(gapNotified["dropped"]) == 0 { + t.Fatalf("the operator signal must carry the skipped app, got %v", gapNotified) + } +} + +// Scenario C — a healthy run is untouched. Without this, "always incomplete" would also pass above, +// and a status that is never green is a status that stops being read. +// +// RED-PROOF: count EVERY skip (drop the classification switch and use len(res.missing)) → a healthy +// run goes amber and this FAILS. +func TestOffboxRun_HealthyRunStaysOk(t *testing.T) { + drive := t.TempDir() + m, sett, prov := classifiedOffboxManager(t, drive) + mkUnit(t, drive, "kept") + prov.hdd["kept"] = drive + prov.has["kept"] = true + _ = sett.SetAppOffbox("kept", true) + + fired := false + m.SetOffboxGapNotify(func(map[string][]string) { fired = true }) + cap := &backupCapture{} + m.SetOffboxRunner(cap.runner()) + if err := m.RunOffboxBackup(context.Background()); err != nil { + t.Fatalf("run: %v", err) + } + got := sett.GetOffboxTarget() + if got.LastStatus != "ok" { + t.Fatalf("LastStatus = %q, want ok — every selected app was carried", got.LastStatus) + } + if fired { + t.Error("the operator signal must NOT fire when nothing was missed") + } + if strings.Contains(got.LastWarning, "NEM kerültek be") { + t.Errorf("a healthy run must carry no skip warning, got %q", got.LastWarning) + } +} + +// Scenario D — a box with NOTHING selected keeps today's behaviour: ok, with the existing +// zero-selection notice. An unconfigured box reporting incomplete forever is its own defect. +// +// RED-PROOF: count the empty selection as a gap → this box goes permanently amber and this FAILS. +func TestOffboxRun_NothingSelectedIsNotAGap(t *testing.T) { + drive := t.TempDir() + m, sett, _ := classifiedOffboxManager(t, drive) + cap := &backupCapture{} + m.SetOffboxRunner(cap.runner()) + if err := m.RunOffboxBackup(context.Background()); err != nil { + t.Fatalf("run: %v", err) + } + got := sett.GetOffboxTarget() + if got.LastStatus != "ok" { + t.Fatalf("LastStatus = %q, want ok — nothing was selected, so nothing was skipped", got.LastStatus) + } + if !strings.Contains(got.LastWarning, "nincs mentésre jelölt alkalmazás") { + t.Errorf("the existing zero-selection notice must survive, got %q", got.LastWarning) + } +} + +// Scenario F — a selected app that is NOT deployed. Decided deliberately: it is NAMED with what to do +// about it, and it does NOT move the verdict, because a box left amber forever by an app somebody +// removed is a status nobody reads. +func TestOffboxRun_SelectedButUndeployedIsNamedNotCounted(t *testing.T) { + drive := t.TempDir() + m, sett, prov := classifiedOffboxManager(t, drive) + mkUnit(t, drive, "kept") + prov.hdd["kept"] = drive + prov.has["kept"] = true + prov.deployed = map[string]bool{"kept": true} // "removed-app" deliberately absent + _ = sett.SetAppOffbox("kept", true) + // selected, no unit, and NOT in the deployed set + _ = sett.SetAppOffbox("removed-app", true) + + fired := false + m.SetOffboxGapNotify(func(map[string][]string) { fired = true }) + cap := &backupCapture{} + m.SetOffboxRunner(cap.runner()) + if err := m.RunOffboxBackup(context.Background()); err != nil { + t.Fatalf("run: %v", err) + } + got := sett.GetOffboxTarget() + if got.LastStatus != "ok" { + t.Fatalf("LastStatus = %q, want ok — an app that is not installed cannot be protected, and must "+ + "not hold the box amber forever", got.LastStatus) + } + if !strings.Contains(got.LastWarning, "removed-app") { + t.Errorf("the undeployed selection must still be NAMED, got %q", got.LastWarning) + } + if !strings.Contains(got.LastWarning, "vedd ki a kijelöl") { + t.Errorf("it must say what to do about it, got %q", got.LastWarning) + } + if fired { + t.Error("an undeployed app must not raise the operator gap signal") + } +} diff --git a/controller/internal/web/offbox_handlers.go b/controller/internal/web/offbox_handlers.go index 7473f1d..83cf33c 100644 --- a/controller/internal/web/offbox_handlers.go +++ b/controller/internal/web/offbox_handlers.go @@ -3,6 +3,7 @@ package web import ( "context" "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -229,12 +230,28 @@ func (s *Server) offboxRunHandler(w http.ResponseWriter, r *http.Request) { offboxRedirect(w, r, "A távoli tároló elárvult — előbb indíts új távoli mentést a kártyán látható módon.", true) return } + // R-234: the single-flight decision is taken SYNCHRONOUSLY, before the goroutine, so the customer + // is told what actually happened to THEIR request. Deciding it inside the goroutine is what made + // the drop invisible: the handler had already answered „elindult" and the page then showed the + // PREVIOUS run's „✓ Rendben". + // IsRunning() is the CONCURRENCY flag — the very one acquireRunning guards — which is what this + // question is about. (The documented "use RestoreStatus for display" trap is a different question.) + if s.backupMgr.IsRunning() { + s.logger.Printf("[INFO] [web] manual off-box backup NOT started for this request: a run is already in flight") + offboxRedirect(w, r, "Már fut egy távoli mentés — ez a kérés nem indított újat. A most látható eredmény még a korábbi futásé; várd meg, míg ez befejeződik.", true) + return + } go func() { ctx, cancel := context.WithTimeout(context.Background(), 3*time.Hour) defer cancel() // ...WithProgress: this is the MANUAL trigger, so the page gets live bytes/percent/current app // (4c). The nightly scheduler keeps calling RunOffboxBackup and stays silent. if err := s.backupMgr.RunOffboxBackupWithProgress(ctx); err != nil { + if errors.Is(err, backup.ErrOffboxRunInFlight) { + // Lost the race between the check above and acquireRunning — rare, and still not a failure. + s.logger.Printf("[INFO] [web] manual off-box backup dropped by the single-flight (raced)") + return + } s.logger.Printf("[WARN] [web] manual off-box backup failed: %v", err) } }() diff --git a/controller/internal/web/offbox_run_inflight_test.go b/controller/internal/web/offbox_run_inflight_test.go new file mode 100644 index 0000000..7be3801 --- /dev/null +++ b/controller/internal/web/offbox_run_inflight_test.go @@ -0,0 +1,71 @@ +package web + +import ( + "bytes" + "log" + "net/http/httptest" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// R-234, THE MEASURED CAUSE — a manual run that the single-flight dropped must not be reported as one +// that started. +// +// WHAT WAS MEASURED (Part 4 venue, 2026-08-06). The customer toggled an app on and pressed +// „Távoli mentés most". The handler answered „A távoli mentés elindult — az állapot itt frissül.", +// `runOffboxBackup` hit `acquireRunning`, logged an INFO and returned **nil**, and the card then +// showed the PREVIOUS run's „✓ Rendben · 1 pillanatkép" — which reads as "the app I just selected is +// backed up". It was not: the restore refused for that app minutes later, and only a third run +// carried it. +// +// The verdict fix (R-234 part 1) does not cover this: there was no skipped app in that run, because +// there was no run. A request that did nothing must say so. +// +// Handler-level on purpose: the decision now lives in the handler, before the goroutine, and a +// manager-level assertion cannot observe what the customer was told. +func TestOffboxRunHandler_InFlightRequestIsNotReportedAsStarted(t *testing.T) { + s, sett, m := newOffboxWebServer(t) + if err := sett.SetOffboxTarget(&settings.OffboxTarget{ + Enabled: true, Host: "nas.local", Port: 22, User: "felhom", RepoPath: "/srv/repo", + Schedule: "daily", EscrowState: "escrowed", + }); err != nil { + t.Fatal(err) + } + if err := m.WriteOffboxSecrets("PRIVATE-KEY-MATERIAL", "nas.local ssh-ed25519 AAAAhostkey"); err != nil { + t.Fatal(err) + } + if !m.OffboxConfigured() || !m.OffboxRunnable() { + t.Fatal("fixture: the target must be configured and runnable, or the handler exits earlier") + } + + // Occupy the single-flight exactly as a run in progress would. + if err := m.AcquireRunningForTest(); err != nil { + t.Fatalf("fixture: %v", err) + } + defer m.ReleaseRunningForTest() + + var logbuf bytes.Buffer + s.logger = log.New(&logbuf, "", 0) + w := httptest.NewRecorder() + s.offboxRunHandler(w, httptest.NewRequest("POST", "/backup/offbox/run", nil)) + + if w.Code != 302 { + t.Fatalf("the handler redirects; got %d", w.Code) + } + loc := w.Header().Get("Location") + if strings.Contains(loc, "elind") { + t.Errorf("a dropped request must NOT be reported as started — that is the defect. Location: %q", loc) + } + if !strings.Contains(loc, "flash_error") { + t.Errorf("it must reach the customer as a problem, not a success flash. Location: %q", loc) + } + // It must also say the visible result belongs to the EARLIER run — that is what was misread. + if !strings.Contains(loc, "kor%C3%A1bbi") { + t.Errorf("the message must say the shown result is the earlier run's. Location: %q", loc) + } + if !strings.Contains(logbuf.String(), "NOT started") { + t.Errorf("the drop must be findable in the log too, got %q", logbuf.String()) + } +}