diff --git a/CHANGELOG.md b/CHANGELOG.md index cee1e83..d4ad15a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,116 @@ +## v0.226.0 — four ways the restore screen could mislead a customer (2026-08-30, R-353/R-357/R-358/R-360) +**MinAgent: 0.129.0** (unchanged — no new agent coupling) + +> **Version note.** The task specifying this work targeted **v0.224.0** against baseline `f8c9390`. +> Both were consumed earlier the same day by R-330 (v0.224.0) and R-331 (v0.225.0). The drift was +> re-confirmed against live Gitea before the first edit, the operator authorised proceeding, and every +> symbol the spec named was re-verified present at the real baseline `e5eee50`. This is that work at +> **v0.226.0**. + +All four defects were proven on `demo-hp` during the 2026-08-21 backup-truth drill and all four were +still in shipped code. They share one acceptance idea: **a restore surface must state what it actually +did, and must refuse what it cannot do.** + +### R-353 — a restore that gave back nothing still said it worked + +`RestoreFromRecoveryUnit` returned only `error`, so the surface reported ` visszaállítva +().` — a sentence equally true of a run that returned an app's entire dataset and of one that +returned nothing. On 2026-08-21 an `opengist` restore printed it over a unit holding `manifest.json` +and `compose/` and nothing else. + +**The count already existed and was thrown away one line deep.** `restoreDockerVolumesFrom` has always +returned it; the wrapper `restoreDockerVolumes` discarded the int. It now returns `(int, error)`, and +`RestoreFromRecoveryUnit` returns `(UnitRestoreResult, error)` carrying volumes replayed, DBs replayed, +**and what the manifest LISTED** — because zero-replayed has two causes and they are opposite news: + +| state | sentence | +|---|---| +| something came back | „A(z) X: 2 adatkötet és az adatbázis visszaállítva — az alkalmazás újraindult." | +| nothing came back, unit listed nothing | „…FIGYELEM: ez a mentés csak a beállításokat tartalmazta, adatot nem." | +| nothing came back, unit listed dumps | „…FIGYELEM — a mentés N adatkötetet és M adatbázis-mentést sorol fel, de egyik sem állt vissza. Az adataid változatlanok maradtak." | + +**Every sentence is a claim about THE BACKUP, never about the app.** „ennek az alkalmazásnak nincs +adata" is forbidden here and the reason is recorded: the off-site twin has `SafetyDump` as an honest +discriminator, this path has none, and 07-backup-architecture §6.3 records that an absent dump has +causes that say nothing about the app (R-361 destroyed apps' canonical `.sql` files for four months). +That is R-355's rule, now extended to the Tier-1 path. + +The snapshot id is dropped from the sentence deliberately: it named WHICH backup ran and said nothing +about what came out of it, which is the question the sentence exists to answer. + +### R-357 — the destructive restore had no free-space gate + +`offbox_reconstitute.go` contained **zero** references to `offboxFree`. All three existing headroom +gates guard NON-destructive paths. The one path that stops the customer's app and overwrites their live +data had none — and on 2026-08-21 it stopped an app, ran out of disk half-way, left 2 of 5 planted +items in place and restarted the app. + +The gate now sits **before `mapOffsiteRestorePaths`, before `writeSafetyDump` and well before +`StopStack`**, so a refusal costs the customer nothing. **Position is the whole fix**, which is why the +test asserts `StopStack` was never called rather than asserting the error string. + +**No headroom multiplier**, matching `PlaceOffsiteRestore`: this is a local copy whose size is known +exactly, unlike `OffboxRestorePrepareFull`'s ×1.1, which is predicting a download. Stated in a comment +so it is not "fixed" later. **Fail-closed on either probe returning ≤ 0** — without that, `free < need` +with `need == 0` is FALSE and an unmeasurable scratch sailed straight through: a gate present and +inert, which is worse than no gate. + +### R-358 — a failed download was offered as a good one + +`OffboxFullScratchReady` answered "the directory exists and is non-empty". A restic run that dies +part-way leaves exactly that, so „Teljes visszaállítás indítása" was offered over a part-copy and +reported success. Its doc comment — *"PlaceOffsiteRestore re-validates per-path completeness"* — is +what made the weak gate look adequate; that call stats top-level placements, not the files inside them. + +`RestoreOffboxScratch` now clears any stale marker **before** restic runs and writes +`.felhom-restore-complete.json` (0600, tmp+fsync+rename) **only after** restic returns nil. The gate +reads it. Absent, unreadable, wrong schema or `full:false` → **not ready**, with a WARN naming which. + +**Both handlers refuse server-side.** The wizard's `PlaceEnabled`/`RestoreEnabled` flags control a +button, and a hidden button is not a guard — a direct POST over a part-copy used to be accepted. + +**Scenario F's open question is answered, and the answer is worse than the question assumed.** The +task asked whether a unit-only scratch is reachable through the real UI flow. **It is, by the most +ordinary route available:** „Ellenőrző visszaállítás" (`mode=unit`, advertised as non-destructive) +writes the SAME directory — `offboxRestoreScratchDir` ignores `full`, and `--include` limits what +restic extracts, never where — so a customer who ran the SAFE verification restore was then offered the +destructive one over a unit-only copy. Filed as **R-396**; the marker closes it. + +### R-360 — the delete refused only while a BACKUP ran + +`offboxVerifyCopyDeleteHandler` guarded on `s.backupMgr.IsRunning()`, which is FALSE for the whole of a +verification restore. Its five siblings on that surface all use `restoreOpBlocked()`, which consults +both flags; this one was missed. **Its doc comment claimed it refused during a restore, and that +sentence is why nobody looked** — it is corrected in place rather than deleted. + +Observed live 2026-08-21 22:35: `RestoreStatus().Running == true` while `IsRunning() == false`, and the +delete of the copy the restore was writing into went through. No app-name comparison was added: +refusing during ANY restore is strictly stronger and matches the other five handlers. + +### One new test seam, and why it was necessary + +`SetOffboxLatestSnapshotFn` overrides the restic snapshot lookup. Without it R-357's gate could not be +tested at the level that matters — reaching it requires getting past `offboxLatestSnapshot`, which +shells out to restic, and a gate proven only by reading the code is the assurance class this project +has been burned by. Nil in production. + +### Red-proofs — each printed the pre-fix behaviour + +| mutation | observed failure | +|---|---| +| revert the outcome to `stackName+" visszaállítva ("+snapshotID+")."` | `THE PRE-FIX SENTENCE REACHED THE CUSTOMER: "opengist visszaállítva (snap-123)."` | +| delete the R-357 headroom gate | `THE APP WAS STOPPED (1 call(s)) … (err=)` on all three gate tests | +| restore the old non-empty scratch check | `a part-copy was reported READY` + the unit-only and unreadable-marker cases | +| revert the delete guard to `IsRunning()` | `THE VERIFICATION COPY WAS DELETED while a restore was writing into it` | +| remove both server-side scratch refusals | both handlers redirected with „elindult" over a part-copy | + +**The first R-357 red-proof exposed a hollow test of my own and is recorded rather than quietly +fixed:** the fixture refused earlier, at the placement stat pre-pass, so `stops == 0` passed against +the pre-fix code. The scratch is now populated the way a completed download leaves it, and the +assertions are ordered so a removed gate reports the outage rather than "no error returned". + +**Green gate:** `go build ./... && go vet ./... && go test ./...` — 28 packages, rc 0. + ## v0.225.0 — the hub could not tell an empty off-site store from an unmeasured one (2026-08-30, R-331) **MinAgent: 0.129.0** (unchanged — no new agent coupling) diff --git a/CONTEXT.md b/CONTEXT.md index e0761a7..c030cc7 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,32 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-08-23 (v0.223.0 — R-329: the alarm reached nobody; R-386: ask the field that knows) +Last updated: 2026-08-30 (v0.226.0 — R-353/R-357/R-358/R-360: the restore tells the truth) + +> **2026-08-30 — v0.226.0. TWO RULINGS THIS SESSION MAKES, recorded so neither is re-litigated.** +> +> **1. A local restore's outcome is a claim about THE BACKUP, never about the app.** This is R-355 +> extended from the off-site path to the Tier-1 path. On the off-site path `SafetyDump` is an honest +> discriminator for "does this app have a database" — a path is returned only when a live database was +> found AND dumped. **The local unit-restore path has no such discriminator at all**, so no claim about +> the app is available to it. „ennek az alkalmazásnak nincs adata" and every variant is forbidden in +> `unitRestoreOutcomeMsg`. It is not merely unproven but unprovable from a manifest: +> 07-backup-architecture §6.3 records that an absent dump has causes that say nothing about the app — +> R-361 destroyed apps' canonical `.sql` files for four months, and a restore in that window would have +> "proven" a database-bearing app had none. +> +> **2. The destructive reconstitute uses NO headroom margin**, matching `PlaceOffsiteRestore` and +> deliberately NOT `OffboxRestorePrepareFull`'s ×1.1. The ×1.1 exists because that gate is *predicting* +> the size of a download it has not made. The reconstitute copies a tree that already exists on disk, +> so its size is measured, not estimated, and a margin over a measured value is a refusal with no fault +> behind it. Stated in a comment at the gate as well, because the two neighbouring gates disagreeing +> looks like an oversight to a reader who does not know which is predicting. +> +> Also this session: three earlier-day releases stacked in front of this one — v0.224.0 (R-330, the +> nightly backup alarming about the apps it was holding down) and v0.225.0 (R-331, `stats_known` on the +> wire). **The task specifying v0.226.0's work was written against v0.223.0 and targeted v0.224.0**; the +> drift was re-confirmed against live Gitea before the first edit rather than assumed. + > **2026-08-23 — v0.223.0 (R-329 + R-386), and a defect that only became visible once another was fixed.** > diff --git a/REUSE.md b/REUSE.md index 5d37891..dd17493 100644 --- a/REUSE.md +++ b/REUSE.md @@ -48,7 +48,12 @@ | `offboxRedirectTo` | controller/internal/web/offbox_handlers.go | `(w, r, page, msg string, isErr bool)` | Same, to an EXPLICIT page | **TRAP (fixed v0.154.0): the separator is chosen, not `"?"`.** Targets may already carry a query — the R-48 wizard is `/backups/restore/app?name=` — and a hardcoded `"?"` buries the flash inside the previous parameter's value | | `restoreOpInFlight` + `hasRecentRestoreResult` | controller/internal/web/restore_wizard.go | `(backup.RestoreOpStatus) bool` / `(st, app, now) bool` | THE "is a restore running / did one just finish" display reads | **TRAP (v0.154.0 shipped this bug): `Manager` has TWO running flags.** `IsRunning()` reads the CONCURRENCY flag, acquired inside the goroutine — and `RestoreOffboxScratch` never acquires it, so it is false for the whole verification restore. Display must read `RestoreStatus().Running` (set synchronously by `BeginRestoreOp`). Read the status ONCE per render or the strip and the suppression can disagree. `hasRecentRestoreResult` is app-bound and window-bounded — a process-wide result must not light another app's „Eredmény" | | `restoreWizardPath` / `deriveWizardStep` / `resolveWizardApp` | controller/internal/web/restore_wizard.go | `(app) string` / `(restoreWizardInput) restoreWizardView` / `([]OffboxAppRow, name) *OffboxAppRow` | R-48 offsite restore wizard: URL builder + the PURE step/unlock derivation + the app-resolution refusals | The step is **never** taken from the request. Precedence is load-bearing: op-running outranks a stale `?full_prep=`, else a commit button reappears mid-restore. Truth table + red-proof: `restore_wizard_test.go`. Adding a form here that posts anywhere new breaks `TestRestoreWizard_NoNewMutationEndpoints` **by design** — R-48 adds no mutation surface | -| `restoreOpBlocked` | controller/internal/web/restore_wizard.go | `() (msg string, blocked bool)` | THE refusal gate before starting ANY restore | **Use this, never a bare `IsRunning()`.** It reads BOTH flags: `RestoreStatus().Running` (set synchronously by `BeginRestoreOp`, true for the whole off-box restore) and `IsRunning()` (the concurrency flag, the only one the nightly backup holds). **R-351: all seven handlers read only `IsRunning()`, which the goroutine acquires AFTER the handler returns — a second press started a second run and was told „…elindult".** Returns the Hungarian refusal, which names the running app and a route | +| `restoreOpBlocked` | controller/internal/web/restore_wizard.go | `() (msg string, blocked bool)` | THE refusal gate before starting ANY restore | **Use this, never a bare `IsRunning()`.** It reads BOTH flags: `RestoreStatus().Running` (set synchronously by `BeginRestoreOp`, true for the whole off-box restore) and `IsRunning()` (the concurrency flag, the only one the nightly backup holds). **R-351: all seven handlers read only `IsRunning()`, which the goroutine acquires AFTER the handler returns — a second press started a second run and was told „…elindult".** Returns the Hungarian refusal, which names the running app and a route | **R-360 (v0.226.0): `offboxVerifyCopyDeleteHandler` was the LAST holdout and its doc comment claimed it already did this — the sentence is why nobody looked. `IsRunning()` is FALSE for the whole of a verification restore, so the copy a restore was writing into could be deleted from the UI (observed live 2026-08-21 22:35). No app-name comparison: refusing during ANY restore is stronger and uniform.** +| `Manager.RestoreFromRecoveryUnit` + `UnitRestoreResult` (R-353, v0.226.0) | controller/internal/backup/restore_unit.go | `(stack string) (UnitRestoreResult, error)` | THE local recovery-unit restore, and the facts its surface must state | **Returns a RESULT, not just an error** — volumes replayed, DBs replayed, and what the manifest LISTED. The Manifest* counts are load-bearing: zero-replayed has two causes (the backup held no data / the backup listed data that did not come back) and they are opposite news. Pair it with `unitRestoreOutcomeMsg`; do NOT write a new sentence. **A claim about the APP is forbidden on this path** — it has no `SafetyDump` discriminator, unlike the off-site twin (CONTEXT.md ruling, 07-backup-architecture §6.3). `restoreDockerVolumes` now returns `(int, error)`; `restoreDockerVolumesFrom` is unchanged and still the shared implementation | +| `unitRestoreOutcomeMsg` + its three message constants (R-353, v0.226.0) | controller/internal/web/handlers.go | `(app string, res backup.UnitRestoreResult) string` | THE customer sentence for a completed LOCAL restore | Twin of `reconstituteOutcomeMsg`; copy its SHAPE (clauses earned by having done the thing, no filesystem path, base names only), never its text. The constants are named because `r353_unit_outcome_test.go` asserts them verbatim — a silent edit is how an honest message drifts back into a comforting one, which is the documented history of the sentence it replaces | +| `offsiteNoSpaceMsgFmt` + `offsiteSizeUnknownMsg` (R-357, v0.226.0) | controller/internal/backup/offbox_restore.go | two consts | EVERY headroom refusal on the off-site restore surface | **All four gates share these** (prepare, scratch, place, and the destructive reconstitute). A customer meeting one wording on one path and a different one on another has to work out whether it is the same problem. **The reconstitute gate uses NO ×1.1 margin** — it copies a measured tree; `OffboxRestorePrepareFull`'s ×1.1 predicts a download. **Fail closed when either probe reads ≤ 0**: `free < need` with `need == 0` is FALSE, so an unmeasurable input sails through — a gate present and inert | +| `Manager.OffboxFullScratchReady` + the scratch marker (R-358, v0.226.0) | controller/internal/backup/offbox_restore.go | `(stack) bool`; `.felhom-restore-complete.json` | THE gate for place-to-live and reconstitute | **It answers "did the run FINISH and was it FULL", not "are there files".** The old non-empty check passed a part-copy from a failed restic run, and the old doc comment ("PlaceOffsiteRestore re-validates per-path completeness") is what made it look adequate — that call stats top-level placements, not files. Marker written 0600 atomically AFTER restic returns nil; stale one cleared BEFORE it starts; both orders pinned by an AST test because `resticStep` is not a seam. Anything else — absent, unreadable, wrong schema, `full:false` — is NOT ready, with a WARN naming which. **Unit-only and full restores write the SAME directory**, so `full` is the only separator (R-396) | +| `Manager.SetOffboxLatestSnapshotFn` (R-357, v0.226.0) | controller/internal/backup/offbox_restore.go | `(fn func(ctx, stack) (id string, paths []string, err error))` INIT/TEST-ONLY | Overriding the restic snapshot lookup in tests | Exists because R-357's gate could not otherwise be tested at the level that matters: reaching it requires getting past `offboxLatestSnapshot`, which shells to restic. **The assertion the seam enables is `StopStack` call count == 0** — a gate placed after the stop returns the right sentence and still takes the outage. Nil in production | | `CheckPlacement` + `PlacementMismatchMessage` | controller/internal/backup/offbox_placement.go | `(*RecoveryManifest, liveDrive, liveNS) PlacementCheck` / `(stack, PlacementCheck) string` | Comparing where a backup SAYS the data lived against where a restore is about to write | Pure and total — nil/empty/blank manifest all give the same honest "not known, no mismatch". **An UNKNOWN is never a mismatch** (refusing on an absence strands every pre-field unit). Compares the DRIVE only (the namespace root is derived from it), Cleaned, so a trailing slash is not a difference. The message names BOTH values on purpose | | `RecordedUnitForStack` + `RecordedAddress` | controller/internal/backup/offbox_placement.go | `(stack) (RecordedPlacement, RecordedAddress, bool)` | Reading back the address + data folder a backup recorded, for a reinstall prefill | Local file reads over every readable namespace root — **no network, no restic, no restore**; it exists for the NOT-INSTALLED case where `GetStackHDDPath` is `""`. **`RecordedAddress.Known()` requires BOTH halves:** an absent `SUBDOMAIN` makes the live deploy path fall back to the CATALOG default (`stacks/deploy.go:88-90`), and offering that back as "what your backup says" is a fabricated fact | | `Metadata.HasDeployField` | controller/internal/stacks/metadata.go | `(envVar string) bool` | "Does this app have somewhere to PUT a recorded value?" | **13 of 53 templates declare `HDD_PATH`; 40 do not** (measured 2026-08-21). For the 40 a recorded placement is a FACT TO STATE, never a value to write into a field that does not exist | diff --git a/controller/README.md b/controller/README.md index 8a4aa89..a7868bc 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1078,6 +1078,31 @@ backups/primary// `mariadb-dump` default `--add-drop-table` — so replay is idempotent). Volume-restore and DB-import failures now **surface** (restore returns an error) instead of a swallowed WARN. Prior to v0.61.0 the per-app restore never replayed the `.sql`, so DB-resident data did not come back. +- **The restore now STATES what came back (v0.226.0, R-353).** `RestoreFromRecoveryUnit` returns + `(UnitRestoreResult, error)` — volumes replayed, DBs replayed, and what the manifest LISTED — and + `unitRestoreOutcomeMsg` (`internal/web/handlers.go`) turns that into the customer's sentence. It + replaces ` visszaállítva ().`, which was equally true of a run that returned an entire + dataset and one that returned nothing; on 2026-08-21 it was printed over a unit holding only + `manifest.json` and `compose/`. Three cases, three sentences: data returned (named and counted); + nothing returned and the unit listed nothing („ez a mentés csak a beállításokat tartalmazta"); nothing + returned though the unit listed dumps („…de egyik sem állt vissza. Az adataid változatlanok + maradtak."). **Every one is a claim about the BACKUP, never about the app** — see CONTEXT.md's ruling + and 07-backup-architecture §6.3. + +### Restore refusals (v0.226.0) + +Three guards added on the off-site restore surface, all server-side: + +| guard | where | refuses when | +|---|---|---| +| **Free space, R-357** | `ReconstituteFromOffsite`, before `mapOffsiteRestorePaths` / `writeSafetyDump` / `StopStack` | the live namespace has less free than the scratch's size. Same wording as the two non-destructive gates (`offsiteNoSpaceMsgFmt`). No headroom multiplier — this copies a measured tree, not a predicted download. **Fail-closed** when either probe reads ≤ 0. The app is never stopped for a refused restore. | +| **Incomplete scratch, R-358** | `offboxPlaceHandler` AND `offboxReconstituteHandler` | `OffboxFullScratchReady` is false — no `.felhom-restore-complete.json`, unreadable, wrong schema, or `full:false`. „A visszaállítási másolat nem teljes…". The wizard flags control a button; these control the operation. | +| **Restore in flight, R-360** | `offboxVerifyCopyDeleteHandler` | any backup **or restore** op is running (`restoreOpBlocked()`, not `IsRunning()`). Previously it refused only during a backup, so the copy a restore was writing into could be deleted from the UI. | + +**The scratch completion marker** (`.felhom-restore-complete.json`, 0600, atomic) is written by +`RestoreOffboxScratch` only after restic returns nil, and any stale one is cleared before restic starts. +It carries `full`, so a unit-only verification restore — which writes the *same* directory — can no +longer unlock the full-restore actions (R-396). #### Tier 2 — off-drive copy (Phase 3, v0.55.x) diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 05ee8e9..cdfa32c 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -144,6 +144,16 @@ type Manager struct { // Windows `go test` host has no `df`). Nil → the real diskFreeBytes (df --output=avail). offboxFreeFn func(path string) int64 + // offboxLatestSnapFn (R-357) overrides the restic snapshot lookup, and it exists for one reason: + // without it, ReconstituteFromOffsite's new headroom gate cannot be tested at the level that + // matters. Scenario D's claim is not "the error string is right" — it is "the app was NEVER + // STOPPED", and reaching the gate at all requires getting past offboxLatestSnapshot, which shells + // out to restic. A test that shells to restic is not a unit test, and a gate proven only by + // reading the code is exactly the class of assurance this project has been burned by. + // + // INIT/TEST ONLY. Nil in production, where offboxLatestSnapshot runs unchanged. + offboxLatestSnapFn func(ctx context.Context, stack string) (string, []string, error) + // F17 restore seams — overridable in tests so the .sql re-import orchestration can be unit-tested // without Docker. Default to the real DiscoverDatabases / ImportDump (lazy-init in reimportDBDumps). discoverDBs func(ctx context.Context) ([]DiscoveredDB, error) diff --git a/controller/internal/backup/offbox_reconstitute.go b/controller/internal/backup/offbox_reconstitute.go index ac3cc27..e4d2f28 100644 --- a/controller/internal/backup/offbox_reconstitute.go +++ b/controller/internal/backup/offbox_reconstitute.go @@ -547,6 +547,44 @@ func (m *Manager) ReconstituteFromOffsite(ctx context.Context, stack string, ack } liveNs := m.namespaceRoot(hdd) + // --- R-357: FREE SPACE, BEFORE ANYTHING IS TOUCHED ------------------------------------------ + // + // This file contained ZERO references to offboxFree until now. The three headroom gates that + // existed all guarded NON-destructive paths (offbox_restore.go: the download sizer, the prepare + // gate, and PlaceOffsiteRestore's missing-only merge). The one path that stops the customer's app + // and overwrites their live data had none. + // + // Measured on demo-hp 2026-08-21: it stopped the app, ran out of disk part-way, left 2 of 5 planted + // items in place and restarted the app — a half-restored dataset presented as a completed restore. + // + // POSITION IS THE WHOLE FIX. This sits before mapOffsiteRestorePaths, before writeSafetyDump and + // well before StopStack, so a refusal costs the customer nothing at all — the app never goes down. + // A gate after StopStack would turn a refusal into an outage, which is the shape it exists to + // prevent. Scenario D asserts the non-effect (StopStack call count 0), not the error string. + // + // NO HEADROOM MULTIPLIER, deliberately, and stated so the next reader does not "fix" it: + // OffboxRestorePrepareFull uses ×1.1 because it is sizing a DOWNLOAD whose final size it is + // predicting. This is a local copy of a tree that already exists on disk, so its size is known + // exactly — the same reasoning PlaceOffsiteRestore's gate uses, and this matches it. + free, need := m.offboxFree()(liveNs), m.offboxSize()(scratch) + switch { + case need <= 0: + // FAIL CLOSED. Without this the comparison below is `free < 0`, which is false, and an + // unmeasurable scratch would sail straight through into the destructive phase — the gate + // present and inert, which is worse than no gate because it reads as protection. + m.logger.Printf("[ERROR] [offbox] %s: REFUSING the destructive restore — the scratch size could not be measured (scratch=%s)", stack, scratch) + return res, fmt.Errorf(offsiteSizeUnknownMsg) + case free <= 0: + // Same direction for the other probe. The customer sentence is shared with the case above + // (the operator asked for one wording); the LOG line above and below is what distinguishes + // which probe failed. + m.logger.Printf("[ERROR] [offbox] %s: REFUSING the destructive restore — free space on the live namespace could not be measured (liveNs=%s)", stack, liveNs) + return res, fmt.Errorf(offsiteSizeUnknownMsg) + case free < need: + m.logger.Printf("[WARN] [offbox] %s: REFUSING the destructive restore — need %d B, free %d B on %s; the app was NOT stopped", stack, need, free, liveNs) + return res, fmt.Errorf(offsiteNoSpaceMsgFmt, humanizeBytes(need), humanizeBytes(free)) + } + placements, err := mapOffsiteRestorePaths(paths, stack, scratch, liveNs) if err != nil { return res, err // whole-placement refusal (no partial writes) diff --git a/controller/internal/backup/offbox_restore.go b/controller/internal/backup/offbox_restore.go index 1b76ba7..b0ede6f 100644 --- a/controller/internal/backup/offbox_restore.go +++ b/controller/internal/backup/offbox_restore.go @@ -30,6 +30,20 @@ const ( // SetOffboxFreeFn overrides the restore free-space probe (tests; the Windows go-test host has no df). func (m *Manager) SetOffboxFreeFn(fn func(path string) int64) { m.offboxFreeFn = fn } +// WriteScratchMarkerForTest exposes the marker writer to the web package's flow test. Test-only by +// name so a production caller reads as obviously wrong: only RestoreOffboxScratch may certify a +// scratch, because only it knows whether the download finished. +func (m *Manager) WriteScratchMarkerForTest(scratch, snapshotID string, full bool) error { + return m.writeScratchMarker(scratch, snapshotID, full) +} + +// SetOffboxLatestSnapshotFn overrides the restic snapshot lookup (tests; no restic needed). See the +// field comment on Manager.offboxLatestSnapFn for why this seam exists rather than a code-reading +// argument that the R-357 gate sits early enough. +func (m *Manager) SetOffboxLatestSnapshotFn(fn func(ctx context.Context, stack string) (string, []string, error)) { + m.offboxLatestSnapFn = fn +} + // SetOffboxFullPlaceCopier overrides the FULL-restore overwrite copier (tests; no rsync needed). func (m *Manager) SetOffboxFullPlaceCopier(fn func(src, dst string) (int, error)) { m.offboxFullPlaceCopier = fn @@ -88,6 +102,9 @@ func offboxUnitPathOf(paths []string, stack string) string { // `snapshots latest --tag --json`. When the tag spans more than one group (old unit-only shape // + new enlarged shape), it returns the newest by time. func (m *Manager) offboxLatestSnapshot(ctx context.Context, stack string) (id string, paths []string, err error) { + if m.offboxLatestSnapFn != nil { + return m.offboxLatestSnapFn(ctx, stack) + } t := m.settings.GetOffboxTarget() base, env := m.offboxBaseArgs(t) sctx, cancel := context.WithTimeout(ctx, offboxProbeTimeout) @@ -239,14 +256,14 @@ func (m *Manager) RestoreOffboxScratch(ctx context.Context, stack string, full b size, serr := m.offboxSnapshotSize(ctx, id) if serr != nil { // SizeUnknown never renders as fits — fail closed. - return fmt.Errorf("A mentés mérete nem állapítható meg — a teljes visszaállítás biztonsági okból nem indítható.") + return fmt.Errorf(offsiteSizeUnknownMsg) } need := size + size/10 // ×1.1 if free < need { - return fmt.Errorf("Nincs elég szabad hely a visszaállításhoz (%s szükséges, %s szabad).", humanizeBytes(need), humanizeBytes(free)) + return fmt.Errorf(offsiteNoSpaceMsgFmt, humanizeBytes(need), humanizeBytes(free)) } } else if free < offboxUnitOnlyFreeFloor { - return fmt.Errorf("Nincs elég szabad hely a visszaállításhoz (%s szükséges, %s szabad).", humanizeBytes(offboxUnitOnlyFreeFloor), humanizeBytes(free)) + return fmt.Errorf(offsiteNoSpaceMsgFmt, humanizeBytes(offboxUnitOnlyFreeFloor), humanizeBytes(free)) } // F-A1 hygiene: drop the legacy rootfs scratch (DataDir/offbox-restore/) best-effort. legacy := filepath.Join(m.cfg.Paths.DataDir, "offbox-restore", stack) @@ -260,6 +277,11 @@ func (m *Manager) RestoreOffboxScratch(ctx context.Context, stack string, full b if err := os.MkdirAll(scratch, 0o755); err != nil { return fmt.Errorf("restore dir: %w", err) } + // R-358: a marker from a PREVIOUS run must never certify this one. Cleared here, before restic + // touches anything, so the window in which a stale certificate could vouch for a part-copy does not + // exist. If this run fails, the scratch is left with files and NO marker — which is precisely the + // state OffboxFullScratchReady must read as "not ready". + m.clearScratchMarker(scratch) t := m.settings.GetOffboxTarget() base, env := m.offboxBaseArgs(t) rctx, cancel := context.WithTimeout(ctx, offboxBackupTimeout) @@ -274,9 +296,94 @@ func (m *Manager) RestoreOffboxScratch(ctx context.Context, stack string, full b return fmt.Errorf("offbox restore %s: %w: %s", stack, rerr, truncate(out)) } m.logger.Printf("[INFO] [offbox] restored %s (%s, full=%v) → %s", stack, id, full, scratch) + // R-358: the completion certificate, written ONLY now — after restic returned nil. Writing it + // earlier would certify a download that has not happened, which is the defect with an extra step. + // Written for full=false runs too: the `full` field inside it, not its presence, is what + // distinguishes a unit-only scratch from a complete one. + if err := m.writeScratchMarker(scratch, id, full); err != nil { + // The restore itself succeeded, so this is not an error to fail the operation on — but it is + // NOT silent, and the consequence is stated: without the marker the scratch reads as not-ready, + // which is the fail-closed direction. Better a re-run than a placement over an uncertified copy. + m.logger.Printf("[ERROR] [offbox] %s: restore succeeded but the completion marker could not be written: %v — the scratch will read as NOT ready and the download must be re-run", stack, err) + } return nil } +// --- R-358: the scratch completion marker ------------------------------------------------------ +// +// THE DEFECT. `OffboxFullScratchReady` used to answer "the directory exists and is non-empty". A restic +// download that failed part-way leaves exactly that: a directory with files in it. So the product +// offered „Teljes visszaállítás indítása" over a part-copy, and pressing it reported success — +// observed on demo-hp 2026-08-21. A non-empty directory is evidence that something was written, never +// that everything was. +// +// The marker is the missing fact: not "are there files" but "did the run that wrote them FINISH, and +// was it the full one". Only the run itself can know that, so only the run writes it. +// +// It lives at the scratch ROOT, which is safe from placement for a reason worth stating rather than +// assuming: `mapOffsiteRestorePaths` builds placements from the SNAPSHOT's own path list, not from a +// directory walk, so a file that exists only locally is invisible to it. That is pinned by +// TestR358_MarkerIsNeverPlaced rather than left as a comment. +const scratchMarkerName = ".felhom-restore-complete.json" + +// scratchMarker is the on-disk completion certificate. `Schema` is carried so a future format change +// is a refusal rather than a misreading — an unrecognised schema fails closed like every other +// unreadable marker. +type scratchMarker struct { + Schema int `json:"schema"` + SnapshotID string `json:"snapshot_id"` + Full bool `json:"full"` + FinishedAt string `json:"finished_at"` +} + +const scratchMarkerSchema = 1 + +// clearScratchMarker removes any existing marker, best-effort. A failure to remove is logged and NOT +// returned: the caller is about to overwrite the scratch anyway, and refusing a restore because a stale +// certificate would not delete trades a real capability for a bookkeeping problem. +func (m *Manager) clearScratchMarker(scratch string) { + if err := os.Remove(filepath.Join(scratch, scratchMarkerName)); err != nil && !os.IsNotExist(err) { + m.logger.Printf("[WARN] [offbox] could not clear the stale scratch marker in %s: %v", scratch, err) + } +} + +// writeScratchMarker writes the certificate atomically (tmp + fsync + rename) at mode 0600. Atomic +// because a torn marker read as valid is the one failure this whole mechanism cannot tolerate — it +// would certify a part-copy, which is the original defect wearing a new hat. +func (m *Manager) writeScratchMarker(scratch, snapshotID string, full bool) error { + data, err := json.Marshal(scratchMarker{ + Schema: scratchMarkerSchema, + SnapshotID: snapshotID, + Full: full, + FinishedAt: time.Now().UTC().Format(time.RFC3339), + }) + if err != nil { + return err + } + final := filepath.Join(scratch, scratchMarkerName) + tmp := final + ".tmp" + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + os.Remove(tmp) + return err + } + if err := f.Sync(); err != nil { + f.Close() + os.Remove(tmp) + return err + } + if err := f.Close(); err != nil { + os.Remove(tmp) + return err + } + return os.Rename(tmp, final) +} + + // OffboxRestorePrepareFull resolves the latest snapshot's restore-size and verifies scratch headroom // for a FULL restore WITHOUT starting it (the two-step size-first gate). Returns the human size on // success, or a Hungarian error to flash on refusal (size unknown / no headroom — fail-closed). @@ -293,7 +400,7 @@ func (m *Manager) OffboxRestorePrepareFull(ctx context.Context, stack string) (s } size, serr := m.offboxSnapshotSize(ctx, id) if serr != nil { - return "", fmt.Errorf("A mentés mérete nem állapítható meg — a teljes visszaállítás biztonsági okból nem indítható.") + return "", fmt.Errorf(offsiteSizeUnknownMsg) } _, nsRoot, derr := m.offboxRestoreScratchDir(stack) if derr != nil { @@ -301,13 +408,37 @@ func (m *Manager) OffboxRestorePrepareFull(ctx context.Context, stack string) (s } need := size + size/10 if free := m.offboxFree()(nsRoot); free < need { - return "", fmt.Errorf("Nincs elég szabad hely a visszaállításhoz (%s szükséges, %s szabad).", humanizeBytes(need), humanizeBytes(free)) + return "", fmt.Errorf(offsiteNoSpaceMsgFmt, humanizeBytes(need), humanizeBytes(free)) } return humanizeBytes(size), nil } -// OffboxFullScratchReady reports whether a (non-empty) full-restore scratch exists for stack — the gate -// for showing the place-to-live action. PlaceOffsiteRestore re-validates per-path completeness. +// R-357 customer-facing refusal strings, shared by every headroom gate on the off-site restore +// surface. Named constants because a test asserts them verbatim and because the destructive gate added +// in v0.226.0 MUST read identically to the two non-destructive ones that predate it — a customer who +// meets this refusal on one path and a differently-worded one on another has to work out whether they +// are the same problem. +const ( + offsiteNoSpaceMsgFmt = "Nincs elég szabad hely a visszaállításhoz (%s szükséges, %s szabad)." + offsiteSizeUnknownMsg = "A mentés mérete nem állapítható meg — a teljes visszaállítás biztonsági okból nem indítható." +) + +// OffboxFullScratchReady reports whether a COMPLETED FULL restore scratch exists for stack — the gate +// for the place-to-live and reconstitute actions. +// +// R-358 — WHAT THIS USED TO ANSWER, AND WHY IT WAS THE WRONG QUESTION. It used to be "the directory +// exists and is non-empty", and its doc comment reassured the reader that +// `PlaceOffsiteRestore re-validates per-path completeness`. That sentence is what made the weak gate +// look adequate, and it is not true in the way it reads: PlaceOffsiteRestore stats the top-level +// PLACEMENTS, not the files inside them, so a placement directory that exists but was only half +// downloaded passes it. A restic run that died part-way leaves a non-empty directory, so the product +// offered „Teljes visszaállítás indítása" over a part-copy and reported success on it (demo-hp, +// 2026-08-21). +// +// It now asks the only question that distinguishes them: did the run that wrote this scratch FINISH, +// and was it the full one. Every other answer — no marker, unreadable marker, wrong schema, full=false +// — is FALSE, and says at WARN which one it was. **Fail closed: an unreadable marker is not a +// completion certificate.** func (m *Manager) OffboxFullScratchReady(stack string) bool { if !isSafeStackName(stack) { return false @@ -319,8 +450,27 @@ func (m *Manager) OffboxFullScratchReady(stack string) bool { if fi, sErr := os.Stat(scratch); sErr != nil || !fi.IsDir() { return false } - entries, _ := os.ReadDir(scratch) - return len(entries) > 0 + data, rErr := os.ReadFile(filepath.Join(scratch, scratchMarkerName)) + if rErr != nil { + if !os.IsNotExist(rErr) { + m.logger.Printf("[WARN] [offbox] %s: scratch completion marker unreadable (%v) — treating the copy as INCOMPLETE", stack, rErr) + } + return false + } + var mk scratchMarker + if uErr := json.Unmarshal(data, &mk); uErr != nil { + m.logger.Printf("[WARN] [offbox] %s: scratch completion marker does not parse (%v) — treating the copy as INCOMPLETE", stack, uErr) + return false + } + if mk.Schema != scratchMarkerSchema { + m.logger.Printf("[WARN] [offbox] %s: scratch completion marker has schema %d, expected %d — treating the copy as INCOMPLETE", stack, mk.Schema, scratchMarkerSchema) + return false + } + if !mk.Full { + m.logger.Printf("[INFO] [offbox] %s: scratch holds a UNIT-ONLY restore (snapshot %s) — not a full copy, so place-to-live stays closed", stack, mk.SnapshotID) + return false + } + return true } // placement is one source→dest pair for place-to-live: src is the reconstructed absolute path under the @@ -432,7 +582,7 @@ func (m *Manager) PlaceOffsiteRestore(ctx context.Context, stack string) error { // F-3a-1b: headroom gate — a missing-only merge copies at most the scratch size; refuse before any // copy if the live drive lacks that (conservative — scratch and live often share a drive). if free, need := m.offboxFree()(liveNs), m.offboxSize()(scratch); free < need { - return fmt.Errorf("Nincs elég szabad hely a visszaállításhoz (%s szükséges, %s szabad).", humanizeBytes(need), humanizeBytes(free)) + return fmt.Errorf(offsiteNoSpaceMsgFmt, humanizeBytes(need), humanizeBytes(free)) } placements, err := mapOffsiteRestorePaths(paths, stack, scratch, liveNs) if err != nil { diff --git a/controller/internal/backup/r357_reconstitute_headroom_test.go b/controller/internal/backup/r357_reconstitute_headroom_test.go new file mode 100644 index 0000000..c255f82 --- /dev/null +++ b/controller/internal/backup/r357_reconstitute_headroom_test.go @@ -0,0 +1,176 @@ +package backup + +import ( + "context" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// ── R-357 — the ONE restore that deletes and replaces data had no free-space gate ──────────────── +// +// The two gentler paths both check (offbox_restore.go: the prepare gate and PlaceOffsiteRestore's +// missing-only merge). `offbox_reconstitute.go` contained ZERO references to offboxFree. +// +// Measured on demo-hp 2026-08-21: the destructive restore stopped the app, ran out of disk part-way, +// left 2 of 5 planted items in place and restarted the app — a half-restored dataset presented as a +// completed restore. +// +// WHAT THESE ASSERT IS THE NON-EFFECT, NOT THE ERROR STRING. The value of the fix is that the app is +// never stopped: a gate placed after StopStack would turn a refusal into an outage and would still +// return the right sentence. So `stops == 0` is the assertion that can fail on a wrong-but-plausible +// implementation, and the message is checked second. + +// r357Provider counts StopStack so the non-effect is measurable, and reports the app as deployed so +// the reconstitute reaches the gate rather than refusing earlier for an unrelated reason. +type r357Provider struct { + hdd string + stops int32 +} + +func (p *r357Provider) GetStackComposePath(string) (string, bool) { return "", false } +func (p *r357Provider) ListDeployedStacks() []StackSummary { + return []StackSummary{{Name: "paperless-ngx"}} +} +func (p *r357Provider) GetStackHDDMounts(string) []string { return nil } +func (p *r357Provider) GetStackHDDPath(string) string { return p.hdd } +func (p *r357Provider) GetImportRoot() string { return "" } +func (p *r357Provider) GetDockerVolumes(string) []string { return nil } +func (p *r357Provider) StopStack(string) error { atomic.AddInt32(&p.stops, 1); return nil } +func (p *r357Provider) StartStack(string) error { return nil } +func (p *r357Provider) RefreshAndIsRunning(string) bool { return true } +func (p *r357Provider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { + return RecoveryInfo{}, false +} +func (p *r357Provider) RecoverStackSecrets(string, []string) map[string]string { return nil } +func (p *r357Provider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { + return nil +} +func (p *r357Provider) StartStackServices(string, []string) error { return nil } +func (p *r357Provider) GetStackClassifiedBinds(string) ([]ClassifiedBind, bool) { + return nil, false +} + +// newR357Manager builds a manager whose reconstitute reaches the headroom gate: a real scratch on +// disk, a stubbed snapshot lookup (no restic), and the app reported deployed. +func newR357Manager(t *testing.T) (*Manager, *r357Provider) { + t.Helper() + m, sett := newOffboxManager(t) + drive := t.TempDir() + if err := sett.AddStoragePath(settings.StoragePath{Path: drive, Label: "drive", Schedulable: true}); err != nil { + t.Fatal(err) + } + prov := &r357Provider{hdd: drive} + m.SetStackProvider(prov) + + scratch, _, err := m.offboxRestoreScratchDir("paperless-ngx") + if err != nil { + t.Fatal(err) + } + + // THE FIXTURE MUST REACH StopStack WHEN THE GATE IS REMOVED, or `stops == 0` proves nothing. + // The first draft of this test did not: without the gate the run refused earlier, at the stat + // pre-pass over the placements, so the assertion passed against the pre-fix code. That is a hollow + // test, and the red-proof is what exposed it — recorded here because the near-miss is the lesson. + // + // So the scratch is populated the way a real completed download leaves it: `mapOffsiteRestorePaths` + // builds each src as filepath.Join(scratch, ), so the snapshot's own absolute + // path is mirrored underneath the scratch. + const oldNs = "/mnt/old" + snapPaths := []string{oldNs + "/backups/primary/paperless-ngx", oldNs + "/appdata/paperless-ngx"} + for _, sp := range snapPaths { + if err := os.MkdirAll(filepath.Join(scratch, sp), 0o755); err != nil { + t.Fatal(err) + } + } + m.SetOffboxLatestSnapshotFn(func(context.Context, string) (string, []string, error) { + return "snap-1", snapPaths, nil + }) + // No Docker in a unit test: the undo copy is seamed out. It runs BEFORE StopStack, so leaving it + // real would make the fixture fail for a reason that has nothing to do with the gate. + m.SetSafetyDumpFn(func(context.Context, DiscoveredDB, string) DumpResult { return DumpResult{} }) + return m, prov +} + +func TestR357_DestructiveRestoreRefusesWithoutHeadroom(t *testing.T) { + m, prov := newR357Manager(t) + m.SetOffboxSizer(func(string) int64 { return 1024 * 1024 }) // the scratch is 1 MB + m.SetOffboxFreeFn(func(string) int64 { return 300 * 1024 }) // 300 KB free + + _, err := m.ReconstituteFromOffsite(context.Background(), "paperless-ngx", false) + + // THE ASSERTION THAT MATTERS, AND IT IS CHECKED FIRST ON PURPOSE. On 2026-08-21 the app went down + // and came back over a half-written dataset. A gate placed after StopStack would return the right + // sentence and still take the outage, so the error text cannot be the primary assertion — and if + // this is checked second, a removed gate reports "no error returned" instead of naming the outage. + if n := atomic.LoadInt32(&prov.stops); n != 0 { + t.Fatalf("THE APP WAS STOPPED (%d call(s)) for a restore with 300 KB free for a 1 MB copy — "+ + "the whole point of this gate is that the customer's app never goes down for a restore "+ + "that cannot run (err=%v)", n, err) + } + if err == nil { + t.Fatal("the destructive restore proceeded with 300 KB free for a 1 MB copy") + } + for _, want := range []string{"Nincs elég szabad hely", "szükséges", "szabad"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal must name need and free like its two siblings; missing %q in %q", want, err.Error()) + } + } +} + +func TestR357_UnknownSizeFailsClosed(t *testing.T) { + // The fail-open hole this closes is subtle: with need = 0 the comparison `free < need` is FALSE, + // so an unmeasurable scratch sailed straight into the destructive phase. A gate that is present + // and inert is worse than no gate, because it reads as protection. + m, prov := newR357Manager(t) + m.SetOffboxSizer(func(string) int64 { return 0 }) + m.SetOffboxFreeFn(func(string) int64 { return 100 << 30 }) // plenty free — irrelevant + + _, err := m.ReconstituteFromOffsite(context.Background(), "paperless-ngx", false) + if n := atomic.LoadInt32(&prov.stops); n != 0 { + t.Fatalf("THE APP WAS STOPPED (%d call(s)) on an unknown size — fail-closed means refuse "+ + "BEFORE the stop, not report an error after it (err=%v)", n, err) + } + if err == nil { + t.Fatal("an unmeasurable scratch was allowed into the destructive restore") + } + if !strings.Contains(err.Error(), "nem állapítható meg") { + t.Errorf("want the size-unknown wording already used by OffboxRestorePrepareFull; got %q", err.Error()) + } +} + +func TestR357_UnknownFreeSpaceFailsClosed(t *testing.T) { + // The mirror hole: free = 0 and need = 0 also compares false. Both probes fail closed. + m, prov := newR357Manager(t) + m.SetOffboxSizer(func(string) int64 { return 1024 }) + m.SetOffboxFreeFn(func(string) int64 { return 0 }) + + _, err := m.ReconstituteFromOffsite(context.Background(), "paperless-ngx", false) + if n := atomic.LoadInt32(&prov.stops); n != 0 { + t.Fatalf("THE APP WAS STOPPED (%d call(s)) on an unknown free reading (err=%v)", n, err) + } + if err == nil { + t.Fatal("an unmeasurable free-space reading was allowed into the destructive restore") + } +} + +func TestR357_AmpleSpaceIsUnchanged(t *testing.T) { + // The gate must not become a new way to fail an ordinary restore. With room to spare it does not + // fire, and the run proceeds past it — which here means it fails LATER, for its own unrelated + // reasons, never with a headroom sentence. + m, _ := newR357Manager(t) + m.SetOffboxSizer(func(string) int64 { return 1024 }) + m.SetOffboxFreeFn(func(string) int64 { return 100 << 30 }) + + _, err := m.ReconstituteFromOffsite(context.Background(), "paperless-ngx", false) + if err != nil && strings.Contains(err.Error(), "Nincs elég szabad hely") { + t.Fatalf("the headroom gate fired with 100 GiB free for a 1 KiB copy: %v", err) + } + if err != nil && strings.Contains(err.Error(), "nem állapítható meg") { + t.Fatalf("the size-unknown branch fired on a measurable scratch: %v", err) + } +} diff --git a/controller/internal/backup/r358_scratch_marker_test.go b/controller/internal/backup/r358_scratch_marker_test.go new file mode 100644 index 0000000..ca8142f --- /dev/null +++ b/controller/internal/backup/r358_scratch_marker_test.go @@ -0,0 +1,248 @@ +package backup + +import ( + "bytes" + "go/ast" + "go/parser" + "go/token" + "log" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// ── R-358 — a failed download was offered as a good one ────────────────────────────────────────── +// +// `OffboxFullScratchReady` used to answer "the directory exists and is non-empty". A restic run that +// dies part-way leaves exactly that. So the product showed „Teljes visszaállítás indítása" over a +// part-copy and pressing it reported success — observed on demo-hp 2026-08-21. +// +// A non-empty directory is evidence that SOMETHING was written, never that everything was. The marker +// carries the only fact that distinguishes them: did the run FINISH, and was it the full one. + +// newR358Manager gives a manager whose scratch resolves into a t.TempDir(). +func newR358Manager(t *testing.T) (*Manager, string) { + t.Helper() + m, sett := newOffboxManager(t) + drive := t.TempDir() + if err := sett.AddStoragePath(settings.StoragePath{Path: drive, Label: "drive", Schedulable: true}); err != nil { + t.Fatal(err) + } + m.SetStackProvider(&offbox3aProvider{ + hdd: map[string]string{"kimai": drive}, binds: map[string][]ClassifiedBind{}, has: map[string]bool{}, + }) + scratch, _, err := m.offboxRestoreScratchDir("kimai") + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(scratch, 0o755); err != nil { + t.Fatal(err) + } + return m, scratch +} + +// writeScratchPayload plants the files a part-way restic run leaves behind. +func writeScratchPayload(t *testing.T, scratch string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(scratch, "backups", "primary", "kimai"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scratch, "backups", "primary", "kimai", "half.tar"), []byte("partial"), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestR358_FailedRestoreLeavesNoUsableScratch(t *testing.T) { + m, scratch := newR358Manager(t) + writeScratchPayload(t, scratch) // files present, run never finished → no marker + + if m.OffboxFullScratchReady("kimai") { + t.Fatal("a part-copy was reported READY — this is the defect: the customer is offered " + + "„Teljes visszaállítás indítása" + " over a download that never finished") + } +} + +func TestR358_UnitOnlyScratchIsNotFullReady(t *testing.T) { + m, scratch := newR358Manager(t) + writeScratchPayload(t, scratch) + if err := m.writeScratchMarker(scratch, "snap-1", false); err != nil { + t.Fatal(err) + } + if m.OffboxFullScratchReady("kimai") { + t.Fatal("a UNIT-ONLY scratch was reported ready for a full restore — both modes write the " + + "same directory, so only the marker's `full` field separates them") + } +} + +func TestR358_StaleMarkerIsClearedBeforeTheRun(t *testing.T) { + m, scratch := newR358Manager(t) + if err := m.writeScratchMarker(scratch, "snap-OLD", true); err != nil { + t.Fatal(err) + } + if !m.OffboxFullScratchReady("kimai") { + t.Fatal("fixture wrong: a valid full marker should read ready") + } + // What the next run does before restic touches anything. + m.clearScratchMarker(scratch) + writeScratchPayload(t, scratch) // ...and then that run dies part-way + + if m.OffboxFullScratchReady("kimai") { + t.Fatal("a marker from a PREVIOUS run certified a later part-copy — the stale certificate " + + "is the whole reason the clear happens before restic, not after") + } +} + +func TestR358_UnreadableMarkerFailsClosed(t *testing.T) { + var buf bytes.Buffer + m, scratch := newR358Manager(t) + m.logger = log.New(&buf, "", 0) + writeScratchPayload(t, scratch) + if err := os.WriteFile(filepath.Join(scratch, scratchMarkerName), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + + if m.OffboxFullScratchReady("kimai") { + t.Fatal("an unparseable marker was treated as a completion certificate") + } + if !strings.Contains(buf.String(), "WARN") { + t.Errorf("a scratch refused for an unreadable marker must say so — silence makes a refusal "+ + "indistinguishable from a missing download; log was %q", buf.String()) + } +} + +func TestR358_WrongSchemaFailsClosed(t *testing.T) { + m, scratch := newR358Manager(t) + writeScratchPayload(t, scratch) + if err := os.WriteFile(filepath.Join(scratch, scratchMarkerName), + []byte(`{"schema":99,"snapshot_id":"s","full":true,"finished_at":"2026-08-30T00:00:00Z"}`), 0o600); err != nil { + t.Fatal(err) + } + if m.OffboxFullScratchReady("kimai") { + t.Fatal("a marker with an unrecognised schema was accepted — a format we cannot read is not a certificate") + } +} + +func TestR358_CompletedFullScratchStillReady(t *testing.T) { + // The happy path is unchanged: a finished full download is still offered. + m, scratch := newR358Manager(t) + writeScratchPayload(t, scratch) + if err := m.writeScratchMarker(scratch, "snap-1", true); err != nil { + t.Fatal(err) + } + if !m.OffboxFullScratchReady("kimai") { + t.Fatal("a COMPLETED full restore is no longer offered — the fix broke the thing it protects") + } +} + +func TestR358_MarkerIsWrittenAt0600AndAtomically(t *testing.T) { + m, scratch := newR358Manager(t) + if err := m.writeScratchMarker(scratch, "snap-1", true); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(filepath.Join(scratch, scratchMarkerName)) + if err != nil { + t.Fatal(err) + } + if fi.Mode().Perm() != 0o600 { + t.Errorf("marker mode = %v, want 0600", fi.Mode().Perm()) + } + // The tmp file must not survive: a leftover .tmp beside the marker is a torn write that a later + // reader could mistake for the real thing. + if _, err := os.Stat(filepath.Join(scratch, scratchMarkerName+".tmp")); !os.IsNotExist(err) { + t.Error("the temporary marker file was left behind") + } +} + +// TestR358_MarkerIsNeverPlaced pins the assumption the whole design rests on: placement is driven by +// the SNAPSHOT's own path list, not by a directory walk, so a file that exists only locally cannot be +// copied into the customer's live data. Stated as a test rather than trusted as a comment — the spec +// asked for exactly this, and "a comment asserting an invariant needs a test pinning it" is a standing +// rule earned nine times over in this project. +func TestR358_MarkerIsNeverPlaced(t *testing.T) { + const stack = "kimai" + oldNs := "/mnt/old" + scratch := t.TempDir() + liveNs := t.TempDir() + snapPaths := []string{ + oldNs + "/backups/primary/" + stack, + oldNs + "/appdata/" + stack, + } + + placements, err := mapOffsiteRestorePaths(snapPaths, stack, scratch, liveNs) + if err != nil { + t.Fatalf("mapOffsiteRestorePaths: %v", err) + } + if len(placements) == 0 { + t.Fatal("fixture produced no placements — the test would prove nothing") + } + for _, pl := range placements { + if strings.Contains(pl.src, scratchMarkerName) || strings.Contains(pl.dst, scratchMarkerName) { + t.Fatalf("the completion marker entered a placement (src=%q dst=%q) — it would be copied "+ + "into the customer's live data", pl.src, pl.dst) + } + } +} + +// TestR358_MarkerIsClearedBeforeResticAndWrittenAfter walks the AST of RestoreOffboxScratch. +// +// It exists because `resticStep` is not a seam — a test cannot run the real download without restic, +// so the ORDER of the three calls cannot be proven by execution here. Order is the entire safety +// property: a marker written before restic certifies a download that has not happened, and a clear +// that runs after it leaves a stale certificate covering a fresh part-copy. A substring search would +// not do: a commented-out call satisfies strings.Contains, which a sibling test in this project +// records paying for. +func TestR358_MarkerIsClearedBeforeResticAndWrittenAfter(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "offbox_restore.go", nil, 0) + if err != nil { + t.Fatalf("parse offbox_restore.go: %v", err) + } + var body *ast.BlockStmt + for _, d := range f.Decls { + if fn, ok := d.(*ast.FuncDecl); ok && fn.Name.Name == "RestoreOffboxScratch" && fn.Body != nil { + body = fn.Body + } + } + if body == nil { + t.Fatal("RestoreOffboxScratch not found") + } + + var order []string + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + switch sel.Sel.Name { + case "clearScratchMarker", "resticStep", "writeScratchMarker": + order = append(order, sel.Sel.Name) + } + } + return true + }) + + idx := func(name string) int { + for i, n := range order { + if n == name { + return i + } + } + return -1 + } + clear, restic, write := idx("clearScratchMarker"), idx("resticStep"), idx("writeScratchMarker") + if clear < 0 || restic < 0 || write < 0 { + t.Fatalf("RestoreOffboxScratch does not call all three (order seen: %v) — the marker is not wired", order) + } + if !(clear < restic) { + t.Errorf("the stale marker is cleared AFTER restic runs (order %v) — a previous run's "+ + "certificate would cover this run's part-copy", order) + } + if !(restic < write) { + t.Errorf("the marker is written BEFORE restic returns (order %v) — that certifies a download "+ + "that has not happened, which is the defect with an extra step", order) + } +} diff --git a/controller/internal/backup/r47_replay_order_test.go b/controller/internal/backup/r47_replay_order_test.go index 446ed40..d96268d 100644 --- a/controller/internal/backup/r47_replay_order_test.go +++ b/controller/internal/backup/r47_replay_order_test.go @@ -164,7 +164,7 @@ func TestReconstituteRefusesWhenNoDBServiceIdentifiable(t *testing.T) { func TestRestoreFromUnitRefusesWhenNoDBServiceIdentifiable(t *testing.T) { m, prov, _ := r47UnitFixture(t, noDBCompose, true) - err := m.RestoreFromRecoveryUnit("app") + _, err := m.RestoreFromRecoveryUnit("app") if err == nil { t.Fatal("expected a refusal: the unit carries a dump but names no startable database service") } @@ -202,7 +202,7 @@ func TestRestoreFromUnitReplaysWithOnlyTheDBServiceUp(t *testing.T) { return nil } - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("restore-from-unit: %v", err) } if len(*imported) != 1 { @@ -227,7 +227,7 @@ func TestRestoreFromUnitReplaysWithOnlyTheDBServiceUp(t *testing.T) { func TestRestoreFromUnitNoDumpsTakesOneFullStart(t *testing.T) { m, prov, imported := r47UnitFixture(t, noDBCompose, false) - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("restore-from-unit: %v", err) } if len(prov.gotServices) != 0 { @@ -251,7 +251,7 @@ func TestRestoreFromUnitIgnoresSafetyDumpsWhenDecidingToReplay(t *testing.T) { mustWrite(t, filepath.Join(AppDBDumpPath(prov.hdd, "app"), preRestoreDumpPrefix+"20260720T101010Z-app-postgres.sql"), pgDump(1)) - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("a lone safety dump must not turn into a refusal: %v", err) } if len(prov.gotServices) != 0 { @@ -327,7 +327,7 @@ func TestRestoreFromUnitReplayFailureStillBringsTheStackUp(t *testing.T) { return context.DeadlineExceeded } - err := m.RestoreFromRecoveryUnit("app") + _, err := m.RestoreFromRecoveryUnit("app") if err == nil { t.Fatal("a failed replay must be surfaced, not swallowed") } diff --git a/controller/internal/backup/restore.go b/controller/internal/backup/restore.go index d0b2566..0454791 100644 --- a/controller/internal/backup/restore.go +++ b/controller/internal/backup/restore.go @@ -64,7 +64,7 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { if m.isDebug() { m.logger.Printf("[DEBUG] RestoreApp: step 2/3 — restoring Docker volumes for %s", stackName) } - if err := m.restoreDockerVolumes(stackName, drivePath); err != nil { + if _, err := m.restoreDockerVolumes(stackName, drivePath); err != nil { m.logger.Printf("[ERROR] RESTORE volume restore failed for %s: %v", stackName, err) dataErr = err } @@ -98,10 +98,18 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { return nil } -// restoreDockerVolumes populates Docker volumes from the tars in the app's LIVE recovery unit. -func (m *Manager) restoreDockerVolumes(stackName, drivePath string) error { - _, err := m.restoreDockerVolumesFrom(stackName, AppVolumeDumpPath(m.namespaceRoot(drivePath), stackName)) - return err +// restoreDockerVolumes populates Docker volumes from the tars in the app's LIVE recovery unit, and +// returns HOW MANY it replayed. +// +// R-353: the count used to be discarded here. `restoreDockerVolumesFrom` has always returned it, so +// the fact existed one call deep and was thrown away one line later — which left the unit-restore +// path structurally unable to tell a customer whether any data came back. On 2026-08-21 an opengist +// restore reported completion over a unit holding manifest.json and compose/ and nothing else, and no +// screen could have said otherwise. Discarding a fact the caller needs is cheaper to fix than to +// re-derive: the caller cannot count volumes afterwards without re-reading the directory the restore +// has already consumed. +func (m *Manager) restoreDockerVolumes(stackName, drivePath string) (int, error) { + return m.restoreDockerVolumesFrom(stackName, AppVolumeDumpPath(m.namespaceRoot(drivePath), stackName)) } // restoreDockerVolumesFrom is restoreDockerVolumes with an EXPLICIT dump directory, and it returns how diff --git a/controller/internal/backup/restore_secrets_gen_test.go b/controller/internal/backup/restore_secrets_gen_test.go index d04c27b..ed4d542 100644 --- a/controller/internal/backup/restore_secrets_gen_test.go +++ b/controller/internal/backup/restore_secrets_gen_test.go @@ -47,7 +47,7 @@ func TestRestoreGeneratesMissingResettableSecret(t *testing.T) { return genValue, true } - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("restore must proceed for a missing RESETTABLE secret: %v", err) } if fake.gotEnv == nil { @@ -90,7 +90,7 @@ func TestRestoreProceedsWhenNoGenerator(t *testing.T) { m.generateSecret = func(string, string) (string, bool) { return "", false } // no spec (Scenario G) } - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("restore must still proceed: %v", err) } if _, present := fake.gotEnv["DB_PASSWORD"]; present { @@ -122,7 +122,7 @@ func TestRestoreGenerationNeverReachesDataKeys(t *testing.T) { return "eager-value", true } - if err := m.RestoreFromRecoveryUnit("app"); err == nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err == nil { t.Fatal("missing data-key must still refuse fail-closed") } if len(genCalls) != 0 { diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index 3c208ac..e6d6dc3 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -129,6 +129,34 @@ func hasReplayableDump(dumpDir string) bool { return false } +// UnitRestoreResult is what a local recovery-unit restore actually did, so the surface can STATE it +// rather than report a bare completion. +// +// It exists for the same reason OffsiteReconstituteResult does, and it is the same lesson arriving on +// the other path: on 2026-08-21 an opengist restore reported "Restore-from-unit completed" over a unit +// holding manifest.json and compose/ and nothing else, and no screen could have told the customer that +// no data had been returned (R-353). +// +// The Manifest* counts are carried BECAUSE zero-replayed has two causes and they are not the same +// fact. A unit that lists no dumps means THE BACKUP held no data. A unit that lists dumps none of which +// replayed means something is wrong and the customer's live data was left untouched. R-355 is the +// standing rule this obeys: a claim about the APP must never be inferred from a counter — and here it +// is not merely unproven but unprovable, because 07-backup-architecture §6.3 records that an app's +// canonical .sql could be absent from the unit for reasons that have nothing to do with whether the app +// has a database (R-361 destroyed exactly that file for four months). +type UnitRestoreResult struct { + // VolumesReplayed is how many named-volume tars were unpacked into live Docker volumes. + VolumesReplayed int + // DBsReplayed is how many .sql dumps were imported. Never inferred from the presence of a database + // service — only a completed import increments it. + DBsReplayed int + // ManifestVolumes is len(manifest.VolumeDumps): what the unit CLAIMS it captured. The gap between + // this and VolumesReplayed is the whole of Scenario C. + ManifestVolumes int + // ManifestDBs is len(manifest.DBDumps): the same claim for the database leg. + ManifestDBs int +} + // RestoreFromRecoveryUnit recreates an app from its on-drive recovery unit. // // It reads the unit manifest, takes the portable secrets from the UNIT and the rest from the guest's @@ -140,15 +168,16 @@ func hasReplayableDump(dumpDir string) bool { // D5: this no longer needs the guest. A restore with the guest's app.yaml absent succeeds, which is // pinned by TestRestoreFromRecoveryUnitWithGuestAbsent — the withheld class is regenerated (O4) and // only a data key missing from BOTH sources still refuses. -func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { +func (m *Manager) RestoreFromRecoveryUnit(stackName string) (UnitRestoreResult, error) { + var res UnitRestoreResult if m.stackProvider == nil { - return fmt.Errorf("stack provider not configured") + return res, fmt.Errorf("stack provider not configured") } m.mu.Lock() if m.running { m.mu.Unlock() - return fmt.Errorf("backup or restore already in progress") + return res, fmt.Errorf("backup or restore already in progress") } m.running = true m.mu.Unlock() @@ -160,7 +189,7 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { drivePath := m.GetAppDrivePath(stackName) if drivePath == "" || !filepath.IsAbs(drivePath) { - return fmt.Errorf("cannot determine drive path for %s", stackName) + return res, fmt.Errorf("cannot determine drive path for %s", stackName) } nsRoot := m.namespaceRoot(drivePath) @@ -170,9 +199,18 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { m.mu.Lock() m.running = false // RestoreApp re-acquires the running flag m.mu.Unlock() - return m.RestoreApp(stackName, "") + // The fallback path has no unit and therefore no manifest to count against: a ZERO result is + // the honest answer, not a missing one. The surface must be able to tell "nothing came back" + // from "we never looked", and it can — ManifestVolumes/ManifestDBs are zero too, which is + // Scenario B's shape and reads as "the backup held no data", which is exactly true of a box + // with no recovery unit. + return res, m.RestoreApp(stackName, "") } + // R-353: what the unit CLAIMS it holds, recorded before any mutation. Read from the manifest that + // was just parsed above, so the claim and the outcome are counted from the same document. + res.ManifestVolumes, res.ManifestDBs = len(manifest.VolumeDumps), len(manifest.DBDumps) + composeDir := RecoveryUnitComposePath(nsRoot, stackName) nonSecretEnv, unitSecrets := readUnitEnv(filepath.Join(composeDir, "app.yaml"), manifest.PortableSecretEnvVars) @@ -185,7 +223,7 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { fullEnv, missing, err := reconcileRestoreSecrets(nonSecretEnv, unitSecrets, guestSecrets, manifest.SecretEnvVars, manifest.DataKeyEnvVars) if err != nil { m.logger.Printf("[ERROR] [backup] Restore REFUSED for %s: %v", stackName, err) - return err + return res, err } // O4: a missing RESETTABLE secret used to redeploy blank (compose "Defaulting to a blank // string" → exit 1). Generate a replacement via the deploy flow's generator instead — @@ -242,7 +280,7 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { hasDumps := hasReplayableDump(AppDBDumpPath(nsRoot, stackName)) if hasDumps && len(dbServices) == 0 { m.logger.Printf("[ERROR] [backup] Restore REFUSED for %s: a .sql dump exists but no database service is identifiable in the unit's compose", stackName) - return fmt.Errorf("Az adatbázis-szolgáltatás nem azonosítható a(z) %s alkalmazásban — a visszaállítás biztonsági okból nem indult el.", stackName) + return res, fmt.Errorf("Az adatbázis-szolgáltatás nem azonosítható a(z) %s alkalmazásban — a visszaállítás biztonsági okból nem indult el.", stackName) } // Stop, restore named-volume data, recreate the definition, replay the DB with ONLY the database @@ -252,12 +290,17 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { if err := m.stackProvider.StopStack(stackName); err != nil { m.logger.Printf("[WARN] [backup] could not stop %s before restore: %v (continuing)", stackName, err) } - if err := m.restoreDockerVolumes(stackName, drivePath); err != nil { - m.logger.Printf("[ERROR] [backup] volume restore for %s: %v", stackName, err) - dataErr = err + // R-353: the count is captured even when the replay errors — a partial replay is a fact the + // customer's sentence has to be built from, and discarding it on the error path is how Scenario C + // would end up wearing Scenario B's wording. + replayed, volErr := m.restoreDockerVolumes(stackName, drivePath) + res.VolumesReplayed = replayed + if volErr != nil { + m.logger.Printf("[ERROR] [backup] volume restore for %s: %v", stackName, volErr) + dataErr = volErr } if err := m.stackProvider.RecreateStackDefinitionFromUnit(stackName, composeDir, fullEnv); err != nil { - return fmt.Errorf("recreating %s from unit: %w", stackName, err) + return res, fmt.Errorf("recreating %s from unit: %w", stackName, err) } // F17: the captured .sql dump is the authoritative logical DB state — replay it AFTER the volume // restore, so the dump WINS over any volume-tar copy of the database. @@ -270,23 +313,27 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { if dataErr == nil { dataErr = err } - } else if _, err := m.reimportDBDumpsCtx(stackName, nsRoot); err != nil { + } else if n, err := m.reimportDBDumpsCtx(stackName, nsRoot); err != nil { + res.DBsReplayed = n // partial credit: whatever imported before the failure really did import m.logger.Printf("[ERROR] [backup] DB re-import for %s: %v", stackName, err) if dataErr == nil { dataErr = err } + } else { + res.DBsReplayed = n } } if err := m.stackProvider.StartStack(stackName); err != nil { - return fmt.Errorf("starting %s after restore from unit: %w", stackName, err) + return res, fmt.Errorf("starting %s after restore from unit: %w", stackName, err) } if err := m.waitForHealthy(stackName, 90*time.Second); err != nil { m.logger.Printf("[WARN] [backup] %s restored but health check failed: %v", stackName, err) } if dataErr != nil { - return fmt.Errorf("restore of %s from unit completed with data errors: %w", stackName, dataErr) + return res, fmt.Errorf("restore of %s from unit completed with data errors: %w", stackName, dataErr) } - m.logger.Printf("[INFO] [backup] Restore-from-unit completed: %s", stackName) - return nil + m.logger.Printf("[INFO] [backup] Restore-from-unit completed: %s — %d volume(s) of %d listed, %d database(s) of %d listed", + stackName, res.VolumesReplayed, res.ManifestVolumes, res.DBsReplayed, res.ManifestDBs) + return res, nil } diff --git a/controller/internal/backup/restore_unit_test.go b/controller/internal/backup/restore_unit_test.go index a57d356..5409ee2 100644 --- a/controller/internal/backup/restore_unit_test.go +++ b/controller/internal/backup/restore_unit_test.go @@ -78,7 +78,7 @@ func TestRestoreFromRecoveryUnitWithGuestAbsent(t *testing.T) { m := &Manager{logger: log.New(io.Discard, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("restore must succeed from the drive alone, got: %v", err) } if fake.gotEnv == nil { @@ -105,7 +105,7 @@ func TestRestoreFromRecoveryUnitGuestAbsentStillFailsClosed(t *testing.T) { m := &Manager{logger: log.New(io.Discard, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} - err := m.RestoreFromRecoveryUnit("app") + _, err := m.RestoreFromRecoveryUnit("app") if err == nil { t.Fatal("expected fail-closed refusal when the data key is in neither source") } @@ -148,7 +148,7 @@ func TestRestoreFromRecoveryUnitOrchestration(t *testing.T) { } m := &Manager{logger: log.New(io.Discard, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("restore: %v", err) } if fake.gotEnv == nil { @@ -169,7 +169,7 @@ func TestRestoreFromRecoveryUnitOrchestration(t *testing.T) { secrets: map[string]string{"DB_PASSWORD": "pw", "SECRET_KEY": "deadbeef"}} m := &Manager{logger: log.New(io.Discard, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} - if err := m.RestoreFromRecoveryUnit("app"); err != nil { + if _, err := m.RestoreFromRecoveryUnit("app"); err != nil { t.Fatalf("a pre-D5 unit must still restore: %v", err) } if fake.gotEnv["SECRET_KEY"] != "deadbeef" { @@ -185,7 +185,7 @@ func TestRestoreFromRecoveryUnitOrchestration(t *testing.T) { } m := &Manager{logger: log.New(io.Discard, "", 0), systemDataPath: filepath.Join(drive, "..", "sys"), stackProvider: fake} - err := m.RestoreFromRecoveryUnit("app") + _, err := m.RestoreFromRecoveryUnit("app") if err == nil { t.Fatal("expected fail-closed refusal, got nil") } diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index cea8922..08eed9d 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -1470,17 +1470,90 @@ func (s *Server) backupRestoreHandler(w http.ResponseWriter, r *http.Request) { start := time.Now() // Phase 2b: restore from the app's recovery unit (recovers secrets from the guest, fail-closed // on an unrecoverable data-encrypting key; falls back to volume-only restore if no unit exists). - if err := s.backupMgr.RestoreFromRecoveryUnit(stackName); err != nil { + res, err := s.backupMgr.RestoreFromRecoveryUnit(stackName) + if err != nil { s.logger.Printf("[ERROR] [web] Restore failed (async): stack=%s: %v", stackName, err) s.backupMgr.EndRestoreOp(false, "Visszaállítás sikertelen: "+err.Error()) return } - s.logger.Printf("[INFO] [web] Restore completed (async): stack=%s in %s", stackName, time.Since(start)) - s.backupMgr.EndRestoreOp(true, stackName+" visszaállítva ("+snapshotID+").") + s.logger.Printf("[INFO] [web] Restore completed (async): stack=%s in %s (volumes %d/%d, dbs %d/%d)", + stackName, time.Since(start), res.VolumesReplayed, res.ManifestVolumes, res.DBsReplayed, res.ManifestDBs) + // R-353: this used to read `stackName+" visszaállítva ("+snapshotID+")."` — a sentence that is + // true of a run which returned an app's entire dataset AND of one that returned nothing at all. + // The customer reads it as "my data is back". The snapshot id is dropped from the sentence + // deliberately: it identified WHICH backup ran and told the customer nothing about what came out + // of it, which is the question the sentence exists to answer. + s.backupMgr.EndRestoreOp(true, unitRestoreOutcomeMsg(stackName, res)) }() http.Redirect(w, r, "/backups/restore?flash="+url.QueryEscape("Visszaállítás elindult — az állapot itt frissül."), http.StatusFound) } +// unitRestoreOutcomeMsg builds the OUTCOME sentence for a completed LOCAL recovery-unit restore. Pure, +// so the wording is unit-testable — this string is the customer's only evidence that the operation did +// what its label promised. +// +// R-353. Modelled on reconstituteOutcomeMsg (the off-site twin), and it is the same defect arriving on +// the Tier-1 path: on 2026-08-21 an opengist restore reported „opengist visszaállítva ()." +// over a unit that held manifest.json and compose/ and nothing else. Every clause below is earned by +// having done the thing, so a restore that really returned data reads exactly as confidently as before. +// +// THE THREE CASES ARE THREE DIFFERENT FACTS, and collapsing any two is the whole bug: +// +// - something came back → name what, and how much. +// - nothing came back AND the unit listed nothing → the BACKUP held only settings. This is a claim +// about the backup, and it is the only claim the manifest can support. +// - nothing came back BUT the unit listed dumps → something is wrong. The customer's live data was +// never removed (the volume replay only ever writes), so say that, and stop them retrying blind. +// +// R-355 IS THE RULE THIS OBEYS AND THE REASON THE MIDDLE CASE IS WORDED AS IT IS. „ennek az +// alkalmazásnak nincs adata" is a claim ABOUT THE APP and must never be inferred from a counter. On the +// off-site path SafetyDump is the honest discriminator for the database question; this path has none, +// so no claim about the app is available here at all. 07-backup-architecture §6.3 is why that is not +// pedantry: R-361 destroyed apps' canonical .sql dumps for four months, so an absent dump has causes +// that have nothing to do with whether the app has a database. +// +// No filesystem path appears in the message, only counts — same rule as reconstituteOutcomeMsg. +func unitRestoreOutcomeMsg(app string, res backup.UnitRestoreResult) string { + if res.VolumesReplayed > 0 || res.DBsReplayed > 0 { + var what string + if res.VolumesReplayed > 0 { + what = fmt.Sprintf("%d adatkötet", res.VolumesReplayed) + } + if res.DBsReplayed > 0 { + if what != "" { + what += " és az adatbázis" + } else { + what = "az adatbázis" + } + } + return fmt.Sprintf(unitRestoreDataMsgFmt, app, what) + } + if res.ManifestVolumes+res.ManifestDBs > 0 { + return fmt.Sprintf(unitRestoreNoneReturnedMsgFmt, app, res.ManifestVolumes, res.ManifestDBs) + } + return fmt.Sprintf(unitRestoreSettingsOnlyMsgFmt, app) +} + +// R-353 customer-facing strings. Named constants, not inlined, because each is asserted verbatim by +// r353_unit_outcome_test.go — a silent edit to any of them is how an honest message drifts back into a +// comforting one, which is the exact history of the sentence they replace. +const ( + // unitRestoreDataMsgFmt — data really came back. %s app, %s the "N adatkötet[ és az adatbázis]" + // clause built above. + unitRestoreDataMsgFmt = "A(z) %s: %s visszaállítva — az alkalmazás újraindult." + + // unitRestoreSettingsOnlyMsgFmt — nothing came back and the unit listed nothing. The FIGYELEM + // sentence is a statement about THE BACKUP; it deliberately says nothing about whether the app has + // data of its own, because the manifest cannot answer that (R-355). + unitRestoreSettingsOnlyMsgFmt = "A(z) %s: a beállítások visszaálltak — az alkalmazás újraindult. FIGYELEM: ez a mentés csak a beállításokat tartalmazta, adatot nem. Az alkalmazás adatai NEM álltak vissza ebből a mentésből." + + // unitRestoreNoneReturnedMsgFmt — the unit listed data and none of it returned. %d volumes, %d + // database dumps LISTED. It states the data is unchanged because that is true and load-bearing: the + // replay only ever writes into volumes, so a replay that did nothing removed nothing, and a customer + // who believes otherwise will do something worse than waiting. + unitRestoreNoneReturnedMsgFmt = "A(z) %s: FIGYELEM — a mentés %d adatkötetet és %d adatbázis-mentést sorol fel, de egyik sem állt vissza. Az adataid változatlanok maradtak. Kérj segítséget, mielőtt újra próbálod." +) + // C9-F1 customer-facing strings. Kept as named constants, not inlined, because both are asserted // verbatim by tests — a silent edit to either is the way an honest message drifts back into a // comforting one. diff --git a/controller/internal/web/offbox_handlers.go b/controller/internal/web/offbox_handlers.go index 2627574..6cc97bd 100644 --- a/controller/internal/web/offbox_handlers.go +++ b/controller/internal/web/offbox_handlers.go @@ -438,6 +438,15 @@ func (s *Server) offboxReconstituteHandler(w http.ResponseWriter, r *http.Reques offboxRedirectTo(w, r, restoreWizardPath(app), msg, true) return } + // R-358: the same server-side refusal as the place handler, and it matters MORE here — this is the + // destructive path. On 2026-08-21 „Teljes visszaállítás indítása" was offered over a part-copy left + // by a failed download and reported ok=true. The wizard's RestoreEnabled flag controls a button; + // this controls the operation. + if !s.backupMgr.OffboxFullScratchReady(app) { + s.logger.Printf("[WARN] [web] off-box reconstitute refused for %s: the restore scratch carries no completion marker", app) + offboxRedirectTo(w, r, restoreWizardPath(app), offsiteScratchIncompleteMsg, true) + return + } // R-351: a SEPARATE field from `confirm`. The restore's own confirm answers "overwrite my live // data"; this one answers "yes, into a different place than the backup recorded". One checkbox // carrying both would be the two-decisions-one-button shape R-48 removed from this surface. @@ -459,6 +468,12 @@ func (s *Server) offboxReconstituteHandler(w http.ResponseWriter, r *http.Reques offboxRedirectTo(w, r, restoreWizardPath(app), "A teljes visszaállítás elindult — az állapot itt frissül.", false) } +// offsiteScratchIncompleteMsg (R-358) is the server-side refusal shown when a place or reconstitute is +// attempted over a scratch that carries no completion marker. Named because two handlers assert it and +// two tests assert it verbatim. It names the action that works — Lane 1 is customer-owned, so a refusal +// that leaves the customer with no next step is not a refusal, it is a dead end. +const offsiteScratchIncompleteMsg = "A visszaállítási másolat nem teljes — a legutóbbi letöltés nem fejeződött be. Indítsd újra a teljes visszaállítás előkészítését." + // reconstituteOutcomeMsg builds the OUTCOME flash for a completed reconstitution. Pure, so the // wording is unit-testable — this string is the customer's only evidence that the operation did // what its label promised, and the zero-file and no-database cases must each read truthfully rather @@ -509,8 +524,15 @@ func reconstituteOutcomeMsg(app string, res backup.OffsiteReconstituteResult) st // `backups/offsite-restore` root it computed itself and refuses anything that lands outside (see // DeleteOffsiteRestoreCopy). The template double-confirms before POSTing. // -// It refuses while a backup/restore op is running: the copy being deleted could be the one currently -// being written. +// R-360 — THE DOC COMMENT USED TO CLAIM THIS AND THE CODE DID NOT DO IT, which is why nobody looked. +// It read "It refuses while a backup/restore op is running"; the guard was `s.backupMgr.IsRunning()`, +// which answers "is a BACKUP running" and is FALSE for the whole of a verification restore (see the +// standing note on the two running flags). Its five siblings on this surface all used +// `restoreOpBlocked()`, which consults both; this one was missed. Observed live 2026-08-21 22:35: +// `RestoreStatus().Running == true` while `IsRunning() == false`, and the delete of the copy the +// restore was writing into went through. +// +// It now refuses while ANY backup or restore op is running. func (s *Server) offboxVerifyCopyDeleteHandler(w http.ResponseWriter, r *http.Request) { if s.backupMgr == nil { offboxRedirectTo(w, r, "/backups/restore", "A mentéskezelő nem érhető el.", true) @@ -526,8 +548,13 @@ func (s *Server) offboxVerifyCopyDeleteHandler(w http.ResponseWriter, r *http.Re offboxRedirectTo(w, r, "/backups/restore", "A törlés megerősítés nélkül nem hajtható végre.", true) return } - if s.backupMgr.IsRunning() { - offboxRedirectTo(w, r, "/backups/restore", "Egy mentési/visszaállítási művelet fut — a törlés most nem biztonságos.", true) + // R-360: `restoreOpBlocked()` and not `IsRunning()`. No app-name comparison is added deliberately: + // refusing during ANY restore is strictly stronger than refusing only for the restoring app, and it + // is the rule the other five handlers on this surface already follow. Uniformity is worth more than + // precision here — the defect was one handler being different. + if msg, blocked := s.restoreOpBlocked(); blocked { + s.logger.Printf("[WARN] [web] verification-copy delete refused for %s: a backup/restore op is running", stack) + offboxRedirectTo(w, r, "/backups/restore", msg, true) return } if err := s.backupMgr.DeleteOffsiteRestoreCopy(stack); err != nil { @@ -556,6 +583,14 @@ func (s *Server) offboxPlaceHandler(w http.ResponseWriter, r *http.Request) { offboxRedirectTo(w, r, restoreWizardPath(app), msg, true) return } + // R-358: refuse SERVER-SIDE over an incomplete scratch. The wizard already hides the button when + // PlaceEnabled is false — and a hidden button is not a guard. This handler is reachable by a POST, + // and before v0.226.0 a POST over a failed download was accepted and reported success. + if !s.backupMgr.OffboxFullScratchReady(app) { + s.logger.Printf("[WARN] [web] off-box place refused for %s: the restore scratch carries no completion marker", app) + offboxRedirectTo(w, r, restoreWizardPath(app), offsiteScratchIncompleteMsg, true) + return + } s.backupMgr.BeginRestoreOp("offbox-place", app) go func() { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) diff --git a/controller/internal/web/r353_unit_outcome_test.go b/controller/internal/web/r353_unit_outcome_test.go new file mode 100644 index 0000000..b796545 --- /dev/null +++ b/controller/internal/web/r353_unit_outcome_test.go @@ -0,0 +1,183 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" + + "io" + "log" + "os" + + "gitea.dooplex.hu/admin/felhom-controller/internal/backup" + "gitea.dooplex.hu/admin/felhom-controller/internal/config" + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// ── R-353 — a local restore that gave back nothing still said it worked ────────────────────────── +// +// Observed on demo-hp 2026-08-21: an `opengist` restore reported „opengist visszaállítva ()." +// over a recovery unit holding manifest.json and compose/ and nothing else. The customer reads that as +// "my data is back". It was not, and no screen in the product could have said so — the count of what +// came back was discarded one line below the function that produced it. +// +// The three cases below are three DIFFERENT facts and collapsing any two is the whole defect. The +// wording of the middle one is constrained by R-355 and by 07-backup-architecture §6.3: it is a claim +// about THE BACKUP, never about the app, because an absent dump has causes that say nothing about +// whether the app has data (R-361 destroyed apps' canonical .sql files for four months). + +func TestUnitRestoreOutcome_VolumesAndDatabaseNamed(t *testing.T) { + msg := unitRestoreOutcomeMsg("kimai", backup.UnitRestoreResult{ + VolumesReplayed: 2, DBsReplayed: 1, ManifestVolumes: 2, ManifestDBs: 1, + }) + for _, want := range []string{"2 adatkötet", "az adatbázis"} { + if !strings.Contains(msg, want) { + t.Errorf("a restore that returned data must NAME it; missing %q in %q", want, msg) + } + } + if strings.Contains(msg, "FIGYELEM") { + t.Errorf("a fully successful restore must not carry a warning; got %q", msg) + } + if strings.Contains(msg, "visszaállítva (") { + t.Errorf("the snapshot-id sentence is the pre-fix shape and says nothing about what came back; got %q", msg) + } +} + +func TestUnitRestoreOutcome_BackupHeldOnlySettings(t *testing.T) { + msg := unitRestoreOutcomeMsg("opengist", backup.UnitRestoreResult{}) + for _, want := range []string{"csak a beállításokat tartalmazta", "NEM álltak vissza"} { + if !strings.Contains(msg, want) { + t.Errorf("a restore that returned no data must say so plainly; missing %q in %q", want, msg) + } + } + // R-355: the forbidden inference. The manifest cannot support a claim about the APP, and on the + // off-site path the equivalent sentence was printed over a live 72-table PostgreSQL. + for _, forbidden := range []string{"nincs adata", "nincs adatbázisa", "alkalmazásnak nincs"} { + if strings.Contains(msg, forbidden) { + t.Fatalf("FALSE CLAIM about the app inferred from a counter (%q) in %q", forbidden, msg) + } + } +} + +func TestUnitRestoreOutcome_ManifestListedDataThatDidNotReturn(t *testing.T) { + msg := unitRestoreOutcomeMsg("paperless-ngx", backup.UnitRestoreResult{ + VolumesReplayed: 0, DBsReplayed: 0, ManifestVolumes: 2, ManifestDBs: 1, + }) + for _, want := range []string{"2 adatkötetet", "1 adatbázis-mentést", "változatlanok maradtak"} { + if !strings.Contains(msg, want) { + t.Errorf("the unit listed data that did not come back — the message must say so; missing %q in %q", want, msg) + } + } + // The Scenario B sentence would say the backup held only settings, which the manifest contradicts. + if strings.Contains(msg, "csak a beállításokat tartalmazta") { + t.Fatalf("wrong case: said the backup held only settings while its manifest lists 3 dumps; got %q", msg) + } +} + +func TestUnitRestoreOutcome_DatabaseOnly(t *testing.T) { + msg := unitRestoreOutcomeMsg("bookstack", backup.UnitRestoreResult{ + VolumesReplayed: 0, DBsReplayed: 1, ManifestVolumes: 0, ManifestDBs: 1, + }) + if !strings.Contains(msg, "az adatbázis visszaállítva") { + t.Errorf("a database-only restore must read naturally; got %q", msg) + } + if strings.Contains(msg, "adatkötet") { + t.Fatalf("named a volume count for a restore that replayed none; got %q", msg) + } + if strings.Contains(msg, "FIGYELEM") { + t.Errorf("data came back — this is not a warning case; got %q", msg) + } +} + +// --- A5: THE SEAM TEST (§10) -------------------------------------------------------------------- +// +// The one that matters. It drives the REAL backupRestoreHandler and reads the sentence off the +// op-status surface the customer's banner polls — not unitRestoreOutcomeMsg directly. Three shipped +// defects in this project came from testing a component whose caller never invoked it, and R-353 is +// itself an instance: restoreDockerVolumesFrom returned the count correctly the whole time. + +type r353Provider struct { + hdd string + starts int32 +} + +func (p *r353Provider) GetStackComposePath(string) (string, bool) { return "", false } +func (p *r353Provider) ListDeployedStacks() []backup.StackSummary { return nil } +func (p *r353Provider) GetStackHDDMounts(string) []string { return nil } +func (p *r353Provider) GetStackHDDPath(string) string { return p.hdd } +func (p *r353Provider) GetImportRoot() string { return "" } +func (p *r353Provider) GetDockerVolumes(string) []string { return nil } +func (p *r353Provider) StopStack(string) error { return nil } +func (p *r353Provider) StartStack(string) error { atomic.AddInt32(&p.starts, 1); return nil } +func (p *r353Provider) RefreshAndIsRunning(string) bool { return true } +func (p *r353Provider) GetStackRecoveryInfo(string) (backup.RecoveryInfo, bool) { + return backup.RecoveryInfo{}, false +} +func (p *r353Provider) RecoverStackSecrets(string, []string) map[string]string { return nil } +func (p *r353Provider) RecreateStackDefinitionFromUnit(string, string, map[string]string) error { + return nil +} +func (p *r353Provider) StartStackServices(string, []string) error { return nil } +func (p *r353Provider) GetStackClassifiedBinds(string) ([]backup.ClassifiedBind, bool) { + return nil, false +} + +func TestR353_HandlerPublishesTheOutcome(t *testing.T) { + tmp := t.TempDir() + lg := log.New(io.Discard, "", 0) + live := filepath.Join(tmp, "live") + if err := os.MkdirAll(live, 0o755); err != nil { + t.Fatal(err) + } + sett, err := settings.Load(filepath.Join(tmp, "settings.json"), lg) + if err != nil { + t.Fatal(err) + } + if err := sett.AddStoragePath(settings.StoragePath{Path: live, Label: "live"}); err != nil { + t.Fatal(err) + } + cfg := &config.Config{} + cfg.Paths.DataDir = tmp + m := backup.NewManager(cfg, sett, lg) + prov := &r353Provider{hdd: live} + m.SetStackProvider(prov) + s := &Server{cfg: cfg, backupMgr: m, logger: lg} + + req := httptest.NewRequest(http.MethodPost, "/backup/restore", + strings.NewReader("stack_name=opengist&snapshot_id=snap-123")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.backupRestoreHandler(w, req) + if w.Code != http.StatusFound { + t.Fatalf("want 302, got %d", w.Code) + } + + // The restore runs in a background goroutine; poll the surface the banner polls. + var last string + for i := 0; i < 900; i++ { + st := m.RestoreStatus() + if !st.Running && st.Last.Message != "" { + last = st.Last.Message + break + } + time.Sleep(10 * time.Millisecond) + } + if last == "" { + t.Fatal("the restore never reached a terminal status") + } + + // There is no recovery unit and no data on this drive, so nothing came back: Scenario B. + if strings.Contains(last, "visszaállítva (snap-123)") { + t.Fatalf("THE PRE-FIX SENTENCE REACHED THE CUSTOMER: %q — it is true of a restore that "+ + "returned an entire dataset and of one that returned nothing", last) + } + for _, want := range []string{"csak a beállításokat tartalmazta", "NEM álltak vissza"} { + if !strings.Contains(last, want) { + t.Fatalf("the published outcome does not state that no data came back; missing %q in %q", want, last) + } + } +} diff --git a/controller/internal/web/r358_r360_handlers_test.go b/controller/internal/web/r358_r360_handlers_test.go new file mode 100644 index 0000000..efa7e12 --- /dev/null +++ b/controller/internal/web/r358_r360_handlers_test.go @@ -0,0 +1,223 @@ +package web + +import ( + "io" + "log" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/backup" + "gitea.dooplex.hu/admin/felhom-controller/internal/config" + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// ── R-358 / R-360 at the HANDLERS ──────────────────────────────────────────────────────────────── +// +// Both defects are server-side. R-358's part-copy was hidden by a template flag, and a hidden button +// is not a guard — these POST directly, which is what a curious customer, a stale tab or a double +// submit does anyway. R-360's delete refused only while a BACKUP ran, so it went through during a +// restore; that one asserts the CONSEQUENCE (the directory still exists), never the branch. + +func newR358Server(t *testing.T) (*Server, *backup.Manager, string) { + t.Helper() + tmp := t.TempDir() + lg := log.New(io.Discard, "", 0) + drive := filepath.Join(tmp, "drive") + if err := os.MkdirAll(drive, 0o755); err != nil { + t.Fatal(err) + } + sett, err := settings.Load(filepath.Join(tmp, "settings.json"), lg) + if err != nil { + t.Fatal(err) + } + if err := sett.AddStoragePath(settings.StoragePath{Path: drive, Label: "drive", Schedulable: true}); err != nil { + t.Fatal(err) + } + if err := sett.SetOffboxTarget(&settings.OffboxTarget{ + Enabled: true, Host: "nas.local", Port: 22, User: "u", RepoPath: "/srv/repo", + Schedule: "daily", EscrowState: "escrowed", + }); err != nil { + t.Fatal(err) + } + cfg := &config.Config{} + cfg.Paths.DataDir = tmp + m := backup.NewManager(cfg, sett, lg) + if err := m.WriteOffboxSecrets("KEY", "nas.local ssh-ed25519 AAAA"); err != nil { + t.Fatal(err) + } + m.SetStackProvider(&r353Provider{hdd: drive}) + s := &Server{cfg: cfg, backupMgr: m, settings: sett, logger: lg} + return s, m, drive +} + +// plantIncompleteScratch writes the exact shape a failed restic download leaves: files, no marker. +func plantIncompleteScratch(t *testing.T, m *backup.Manager, app string) string { + t.Helper() + scratch := m.OffsiteRestoreScratchPath(app) + if scratch == "" { + t.Fatal("could not resolve the scratch path") + } + if err := os.MkdirAll(filepath.Join(scratch, "backups", "primary", app), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scratch, "backups", "primary", app, "half.tar"), []byte("partial"), 0o644); err != nil { + t.Fatal(err) + } + return scratch +} + +func TestR358_PlaceHandlerRefusesIncompleteScratch(t *testing.T) { + s, m, _ := newR358Server(t) + plantIncompleteScratch(t, m, "kimai") + + req := httptest.NewRequest(http.MethodPost, "/backup/offbox/place", strings.NewReader("app=kimai")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.offboxPlaceHandler(w, req) + + loc := w.Header().Get("Location") + if !strings.Contains(loc, "nem+teljes") && !strings.Contains(loc, "nem%20teljes") { + t.Fatalf("a direct POST over a part-copy was NOT refused server-side; redirect was %q", loc) + } + if m.RestoreStatus().Running { + t.Fatal("the place operation actually STARTED over an incomplete scratch") + } +} + +func TestR358_ReconstituteHandlerRefusesIncompleteScratch(t *testing.T) { + // The destructive one. On 2026-08-21 „Teljes visszaállítás indítása" was offered over exactly this + // state and reported ok=true. + s, m, _ := newR358Server(t) + plantIncompleteScratch(t, m, "kimai") + + req := httptest.NewRequest(http.MethodPost, "/backup/offbox/reconstitute", + strings.NewReader("app=kimai&confirm=1")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.offboxReconstituteHandler(w, req) + + loc := w.Header().Get("Location") + if !strings.Contains(loc, "nem+teljes") && !strings.Contains(loc, "nem%20teljes") { + t.Fatalf("the DESTRUCTIVE restore was not refused over a part-copy; redirect was %q", loc) + } + if m.RestoreStatus().Running { + t.Fatal("the destructive restore actually STARTED over an incomplete scratch") + } +} + +// TestR360_VerifyCopyDeleteRefusedDuringRestore — Scenario G, asserting the CONSEQUENCE. +// +// The state is the one observed live on 2026-08-21 22:35 and it is the whole reason the bug existed: +// RestoreStatus().Running is TRUE while IsRunning() is FALSE. The old guard read only the second. +func TestR360_VerifyCopyDeleteRefusedDuringRestore(t *testing.T) { + s, m, drive := newR358Server(t) + + // A real verification copy on disk, at the path DeleteOffsiteRestoreCopy resolves. + copyDir := filepath.Join(drive, "backups", "offsite-restore", "kimai") + if err := os.MkdirAll(copyDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(copyDir, "payload.txt"), []byte("the copy a restore is writing into"), 0o644); err != nil { + t.Fatal(err) + } + + // The live state: a restore op in flight, no BACKUP running. + m.BeginRestoreOp("offbox-restore", "kimai") + if !m.RestoreStatus().Running { + t.Fatal("fixture wrong: no restore op is in flight") + } + if m.IsRunning() { + t.Fatal("fixture wrong: IsRunning() must be FALSE — that divergence IS the defect") + } + + req := httptest.NewRequest(http.MethodPost, "/backup/offbox/verify-copy/delete", + strings.NewReader("stack=kimai&confirm=1")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.offboxVerifyCopyDeleteHandler(w, req) + + // THE ASSERTION THAT MATTERS: the copy the restore is writing into is still there. + if _, err := os.Stat(copyDir); os.IsNotExist(err) { + t.Fatal("THE VERIFICATION COPY WAS DELETED while a restore was writing into it — this is " + + "the 2026-08-21 behaviour, and the customer can do it from the UI") + } + if _, err := os.Stat(filepath.Join(copyDir, "payload.txt")); err != nil { + t.Fatalf("the copy's contents did not survive the delete attempt: %v", err) + } + if loc := w.Header().Get("Location"); !strings.Contains(loc, "flash_error") { + t.Errorf("the refusal must reach the customer as an error flash; redirect was %q", loc) + } +} + +func TestR360_VerifyCopyDeleteStillWorksWhenIdle(t *testing.T) { + // Scenario H for this handler: with nothing in flight the delete must still work. A guard that + // refuses always is not a fix, it is a removed feature. + s, _, drive := newR358Server(t) + copyDir := filepath.Join(drive, "backups", "offsite-restore", "kimai") + if err := os.MkdirAll(copyDir, 0o755); err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest(http.MethodPost, "/backup/offbox/verify-copy/delete", + strings.NewReader("stack=kimai&confirm=1")) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.offboxVerifyCopyDeleteHandler(w, req) + + if _, err := os.Stat(copyDir); !os.IsNotExist(err) { + t.Fatalf("an idle delete no longer removes the copy (redirect %q)", w.Header().Get("Location")) + } +} + +// TestR358_UnitOnlyScratchClosesTheFullRestoreCard — Scenario F at the FLOW level. +// +// THE ANSWER TO THE SPEC'S OPEN QUESTION, and it is worse than the question assumed. The task asked +// whether the real UI flow can reach a state where a unit-only scratch makes the full-restore action +// appear. It can, and by the MOST ORDINARY route available: +// +// - „Ellenőrző visszaállítás" (`mode=unit`, the default, advertised as non-destructive) calls +// RestoreOffboxScratch(ctx, app, full=false); +// - both modes write the SAME directory — offboxRestoreScratchDir ignores `full`, and `--include` +// limits WHAT restic extracts, never WHERE; +// - the wizard sets ScratchReady from OffboxFullScratchReady, which pre-fix answered +// "directory exists and is non-empty"; +// - deriveWizardStep then sets PlaceEnabled AND RestoreEnabled from that one flag. +// +// So a customer who ran the SAFE verification restore was then offered „Teljes visszaállítás +// indítása" over a unit-only copy. Filed as a register row; the fix closes it because the marker +// records full=false. +func TestR358_UnitOnlyScratchClosesTheFullRestoreCard(t *testing.T) { + s, m, _ := newR358Server(t) + scratch := m.OffsiteRestoreScratchPath("kimai") + if err := os.MkdirAll(scratch, 0o755); err != nil { + t.Fatal(err) + } + // What a completed `mode=unit` verification restore leaves behind. + if err := os.MkdirAll(filepath.Join(scratch, "mnt", "old", "backups", "primary", "kimai"), 0o755); err != nil { + t.Fatal(err) + } + if err := m.WriteScratchMarkerForTest(scratch, "snap-1", false); err != nil { + t.Fatal(err) + } + + ready := s.backupMgr.OffboxFullScratchReady("kimai") + if ready { + t.Fatal("a unit-only verification restore still unlocks the full-restore card — the customer " + + "is offered a destructive restore over a copy that holds only the recovery unit") + } + view := deriveWizardStep(restoreWizardInput{App: "kimai", ScratchReady: ready}) + if view.RestoreEnabled || view.PlaceEnabled { + t.Fatalf("the wizard still offers place/restore over a unit-only scratch: %+v", view) + } + if !view.PrepareEnabled { + t.Fatal("the customer is left with no way forward — PrepareEnabled must be true so they can " + + "run the real full download") + } + if !view.VerifyEnabled { + t.Fatal("the verification restore must stay available") + } +}