0d52a42c17
gates / gates (push) Successful in 12s
Nothing ever verified that the off-site copies are still readable. The whole-guest tier has verify jobs; the tier holding the customer's documents and photos had none -- the complete set of restic verbs this controller used contained no `check`. We would have found out at restore time, with a customer waiting. On 2026-08-21 a deliberately damaged pack was caught at once by plain `restic check`; we had never run it. R-397: NotifyIntegrityOK/NotifyIntegrityFailed existed with no caller, the hub allowlists both event types and carries the Hungarian text for both, the settings checkbox exists, and the debug button posts to /api/debug/backup/ integrity. Everything was built except the part that runs. SIXTH instance of that shape in this project. THE HAZARD SHAPES THE WHOLE DESIGN. resticStep self-heals a crash lock by running `unlock --remove-all` and retrying, and its own comment records why that is safe: every caller holds the in-process single-flight mutex, so any lock it meets is stale. A check that did not take that flag could meet a LIVE prune's lock from this same box, remove it, and retry over the top of it. So the check TAKES THE FLAG and SKIPS rather than waits -- waiting would pin the nightly backup behind it, and a skip costs nothing because due-ness makes tomorrow try again. TestR359_SkipsWhenRunningFlagHeld asserts the NON-EFFECTS: restic never invoked, `unlock` never in any argv. Its red-proof prints the real thing -- restic running `check` while the flag was held. DUE-NESS, NOT A WEEKDAY. Daily job, weekly behaviour: "is the last successful check older than 7 days?" not "is it Sunday?". R-341 is exactly the other shape, a dated check quietly missed and never caught up. No Weekly primitive added. THREE OUTCOMES, NOT TWO. Skipped, Unreachable and failed are different facts. "I could not look" is not "I looked and it is broken" -- R-339 already owns reachability, and a second alarm for the same fact trains the operator to discount the one alarm that means the backups are damaged. A timeout is unreachable, never damage. A failure advances due-ness (a broken store must not be re-checked nightly); a skip and an unreachable store do not. Success is severity `info`, which severityNotifies DROPS -- it mails NOBODY, by design. A weekly success e-mail is how people stop reading their alerts. The customer gets a SENTENCE; restic's words go to the log, truncated (R-379: 615 bytes of raw database text reached a customer once). read-data-subset ships OFF and a malformed value is refused at read time rather than handed to restic, where one typo would fail the whole check. Published on OffboxReportStatus, NOT on report.BackupReport's IntegrityOK -- those were retired by R-331 YESTERDAY and TestBackupReport_DeadFieldsStayZero still passes unmodified. Also: the monitoring page stopped promising a Sunday job that never existed, and the debug button got its dispatch case. PART 0 WAS NOT BUILT, AND R-398 WAS MY OWN MISTAKE. The seam it asked for already exists: offboxRunner/SetOffboxRunner/m.runner() has been injectable since the off-site tier shipped, and other tests drive restic-backed paths through it. A resticStepFn seam would have been WORSE here -- it would replace the `unlock --remove-all` escalation and hide it from the assertions that must see it. R-358's AST ordering test is converted to a real execution test instead, which immediately surfaced something the AST walk could not: unlockStale legitimately runs before the restore. Four red-proofs, each printing the pre-fix behaviour. Green gate: 28 packages, rc 0. All 12 controller gates OK.
308 lines
12 KiB
Go
308 lines
12 KiB
Go
package backup
|
|
|
|
import (
|
|
"bytes"
|
|
"context"
|
|
"encoding/json"
|
|
"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_MarkerOrderingIsExecuted — R-398, and the row that prompted it was WRONG in a way worth
|
|
// recording rather than quietly fixing.
|
|
//
|
|
// This test used to walk the AST of RestoreOffboxScratch, on the stated ground that "`resticStep` is
|
|
// not a seam, so the ORDER cannot be proven by execution". **The first half is true and the conclusion
|
|
// was false.** `resticStep` is not overridable, but the layer it calls — `offboxRunner`, injected by
|
|
// `SetOffboxRunner` — has been a seam since the off-site tier shipped, and other tests in this package
|
|
// have been driving restic-backed paths through it all along. R-398 was filed off my own mistaken
|
|
// reading; it is corrected, not closed.
|
|
//
|
|
// Executing it is strictly stronger than the AST walk, and not only because it runs the real function:
|
|
// the runner sees EVERY argv, including the `unlock --remove-all` escalation that `resticStep` performs
|
|
// on a lock error. A `resticStepFn` seam — which R-398 proposed and this test's existence argued for —
|
|
// would have REPLACED that escalation and hidden it from exactly the assertions that need to see it.
|
|
//
|
|
// The order is the whole 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 over a fresh part-copy.
|
|
func TestR358_MarkerOrderingIsExecuted(t *testing.T) {
|
|
m, scratch := newR358Manager(t)
|
|
// A stale marker from a previous run, so "cleared before" is observable as a state change rather
|
|
// than as an absence that was always absent.
|
|
if err := m.writeScratchMarker(scratch, "snap-OLD", true); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
var events []string
|
|
markerAtRestic := "unset"
|
|
m.SetOffboxLatestSnapshotFn(func(context.Context, string) (string, []string, error) {
|
|
return "snap-NEW", []string{"/mnt/old/backups/primary/kimai"}, nil
|
|
})
|
|
m.SetOffboxFreeFn(func(string) int64 { return 100 << 30 })
|
|
m.SetOffboxRunner(func(_ context.Context, _ []string, args ...string) ([]byte, error) {
|
|
switch {
|
|
case containsArg(args, "cat") && containsArg(args, "config"):
|
|
return []byte(`{"version":2}`), nil
|
|
case containsArg(args, "stats"):
|
|
return []byte(`{"total_size":1024}`), nil
|
|
case containsArg(args, "restore"):
|
|
events = append(events, "restic-restore")
|
|
// THE OBSERVATION: what does the marker look like at the moment restic runs? If the clear
|
|
// happened first it is gone; if the write happened first it is already there, certifying a
|
|
// download that has not finished.
|
|
if _, err := os.Stat(filepath.Join(scratch, scratchMarkerName)); err == nil {
|
|
markerAtRestic = "present"
|
|
} else {
|
|
markerAtRestic = "absent"
|
|
}
|
|
return nil, nil
|
|
case containsArg(args, "unlock"):
|
|
// TWO DIFFERENT UNLOCKS, and only one of them is dangerous. Plain `unlock` is restic's
|
|
// stale-ONLY sweep, run as pre-run hygiene by unlockStale before every restore.
|
|
// `unlock --remove-all` is resticStep's crash-lock escalation, and it is the act that must
|
|
// never happen beside a live sibling operation. Recording them separately here is what
|
|
// lets the R-359 lock-safety tests assert the second is absent without tripping over the
|
|
// first — a distinction the AST walk this replaced could not have surfaced at all.
|
|
if containsArg(args, "--remove-all") {
|
|
events = append(events, "unlock--remove-all")
|
|
} else {
|
|
events = append(events, "unlock-stale")
|
|
}
|
|
return nil, nil
|
|
}
|
|
return nil, nil
|
|
})
|
|
|
|
if err := m.RestoreOffboxScratch(context.Background(), "kimai", true); err != nil {
|
|
t.Fatalf("RestoreOffboxScratch: %v", err)
|
|
}
|
|
|
|
if markerAtRestic != "absent" {
|
|
t.Fatalf("the completion marker was %s while restic was still downloading — a marker that "+
|
|
"predates the download certifies a copy that does not exist yet (and here it was the "+
|
|
"PREVIOUS run's marker, covering a fresh part-copy)", markerAtRestic)
|
|
}
|
|
// The restore must actually have run, or the ordering assertion above proves nothing. It is NOT
|
|
// asserted to be first: `unlockStale` legitimately precedes it as pre-run hygiene, which this
|
|
// execution test surfaced on its first run and the AST walk could never have shown.
|
|
if !containsEvent(events, "restic-restore") {
|
|
t.Fatalf("restic never ran, so this test proves nothing about ordering (events=%v)", events)
|
|
}
|
|
// And the DANGEROUS unlock never appeared: nothing here met a lock, so resticStep never escalated.
|
|
if containsEvent(events, "unlock--remove-all") {
|
|
t.Fatalf("`unlock --remove-all` ran during an ordinary restore (events=%v) — that escalation "+
|
|
"is only safe because the single-writer flag proves no sibling is live", events)
|
|
}
|
|
// And after a successful run the marker is there, carrying THIS run's snapshot, not the old one.
|
|
data, err := os.ReadFile(filepath.Join(scratch, scratchMarkerName))
|
|
if err != nil {
|
|
t.Fatalf("no marker after a successful restore: %v", err)
|
|
}
|
|
var mk scratchMarker
|
|
if err := json.Unmarshal(data, &mk); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if mk.SnapshotID != "snap-NEW" || !mk.Full {
|
|
t.Fatalf("marker = %+v, want snapshot snap-NEW and full=true — the stale one survived", mk)
|
|
}
|
|
if !m.OffboxFullScratchReady("kimai") {
|
|
t.Fatal("a completed full restore did not read as ready")
|
|
}
|
|
}
|
|
|
|
func containsEvent(ev []string, want string) bool {
|
|
for _, e := range ev {
|
|
if e == want {
|
|
return true
|
|
}
|
|
}
|
|
return false
|
|
}
|
|
|
|
// containsArg is a local helper so this file does not depend on another test file's ordering.
|
|
func containsArg(args []string, want string) bool {
|
|
for _, a := range args {
|
|
if a == want {
|
|
return true
|
|
}
|
|
}
|
|
return false
|
|
}
|