From e000e201af8edc00e94d9c951c7f3cf265525889 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 28 Jul 2026 13:12:37 +0200 Subject: [PATCH] R-100: record the offsite last-SUCCESS anchor (v0.181.0) LastRun records an attempt, not a result. New OffboxTarget.LastSuccess, set only on the success branch via the pure offboxAnchorAfterRun rule, carried to the hub as last_success. Closes two silent-wipe sites (settings save, hub re-apply). --- CHANGELOG.md | 39 ++++++ controller/internal/backup/offbox.go | 43 +++++- .../backup/offbox_lastsuccess_test.go | 132 ++++++++++++++++++ controller/internal/settings/settings.go | 17 ++- controller/internal/web/offbox_handlers.go | 4 + .../internal/web/offbox_lastsuccess_test.go | 69 +++++++++ 6 files changed, 298 insertions(+), 6 deletions(-) create mode 100644 controller/internal/backup/offbox_lastsuccess_test.go create mode 100644 controller/internal/web/offbox_lastsuccess_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cccbe5..76fcf5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,44 @@ ## Changelog +### v0.181.0 — R-100: record the last SUCCESS, not just the last attempt (2026-07-28) + +The producer half of R-100. `OffboxTarget` gains **`LastSuccess`** (RFC3339), carried to the hub on the +offsite report as `last_success`. The hub's staleness verdict counts from it (hub v0.80.0). + +**Why a new field rather than reading `LastStatus`.** `LastRun` is written unconditionally at the end +of every run, failures included — it records an **attempt**. "How long since `LastRun`" therefore +answers "how long since we last TRIED", which is not the question a freshness verdict asks. The +alternative — "`LastStatus == error` ⇒ stale" — turns every transient blip into an immediate alarm, +which is the F-A1 noise failure mode. Anchoring on last success tolerates one bad night and catches a +persistent one, using the threshold that already exists. + +The rule is a pure function, `offboxAnchorAfterRun(prev, at, runErr)`, called unconditionally beside +the `LastRun` write. Both directions are bugs if got wrong and both are pinned: +- a failure must not **advance** it → or the original defect survives; +- a failure must not **clear** it → or one bad night makes an established tier read as never-succeeded + (the mirror-image over-correction, and on the hub side the newborn-box path). + +**Two silent-wipe sites found and closed**, both of the "seam built but never wired" shape — the field +exists, the writer sets it, and an unrelated routine path zeroes it: +- `offboxConfigHandler` rebuilds the target from the form and copies runtime status field by field, so + an ordinary settings save (edit the host, edit the path) would have erased the anchor; +- `ApplyOffsiteTarget` does the same on a hub re-apply — an established tier reset to "never + succeeded" every time the hub re-pushed its descriptor. + +Neither would have surfaced until the hub's verdict changed, days later. + +**A hollow test of my own, caught by red-proofing it.** The first version of +`TestOffboxLastSuccess_OnlyAdvancesOnSuccess` re-implemented the rule in a local closure: mutating the +production code left it **green**. That is what the extraction to `offboxAnchorAfterRun` is for — the +test now calls the real rule, and the red-proof bites. + +Red-proofs, all observed failing: drop the `runErr` guard → `a FAILED run advanced LastSuccess to +"2026-07-21T02:15:00Z" — that is the R-100 defect in mirror image`; always return `prev` → `a +successful run did not advance the anchor`; drop the wire field → `OffboxReportStatus dropped +LastSuccess — the hub would degrade forever on a controller that has it`; drop the handler +preservation → `a settings save erased LastSuccess`. + + ### v0.180.0 — F-OBS: the dead-app check gets a positive observable (2026-07-28) On a default `logging.level: info` box there was **no way to tell whether `deadapp-check` had run**. diff --git a/controller/internal/backup/offbox.go b/controller/internal/backup/offbox.go index 6cd496f..884a00d 100644 --- a/controller/internal/backup/offbox.go +++ b/controller/internal/backup/offbox.go @@ -363,6 +363,10 @@ func (m *Manager) ApplyOffsiteTarget(ctx context.Context, tgt *settings.OffboxTa tgt.EscrowState = cur.EscrowState tgt.LastRun, tgt.LastStatus, tgt.LastError = cur.LastRun, cur.LastStatus, cur.LastError tgt.LastDuration, tgt.LastWarning = cur.LastDuration, cur.LastWarning + // R-100: carry the staleness anchor across a hub re-apply, for the same reason as the rest of + // this block — a re-apply is not a new tier. Dropping it would reset an established tier to + // "never succeeded" every time the hub re-pushes the descriptor. + tgt.LastSuccess = cur.LastSuccess tgt.RepoSizeHuman, tgt.RepoSizeBytes, tgt.SnapshotCount = cur.RepoSizeHuman, cur.RepoSizeBytes, cur.SnapshotCount } if tgt.EscrowState != "escrowed" { @@ -714,6 +718,10 @@ func (m *Manager) runOffboxBackup(ctx context.Context, withProgress bool) error } if perr := m.settings.UpdateOffboxStatus(func(o *settings.OffboxTarget) { o.LastRun = time.Now().UTC().Format(time.RFC3339) + // R-100: LastRun above records the ATTEMPT; this records the RESULT. The hub's staleness + // verdict counts from the anchor, never from the attempt. INVARIANT: a failed run neither + // advances nor clears it — pinned by TestOffboxAnchorAfterRun_* , not asserted in prose. + o.LastSuccess = offboxAnchorAfterRun(o.LastSuccess, o.LastRun, runErr) o.LastDuration = dur.Round(time.Second).String() if errors.Is(runErr, ErrOffboxOrphaned) { // First-detection of the orphaned repo: RepoState (set by markOrphaned) drives the orphan @@ -994,13 +1002,39 @@ func (m *Manager) runOffboxInternal(ctx context.Context, apps, base, env []strin // multiplied by the retained-snapshot count (SP-1). The displayed size drops one-time after deploy. const offboxGiB = int64(1) << 30 +// offboxAnchorAfterRun returns the last-SUCCESS anchor after a run that finished at `at` with +// `runErr`, given the anchor value `prev` from before the run. R-100. +// +// THE RULE THIS ENCODES: a timestamp recording an ATTEMPT is not evidence of a RESULT. `LastRun` is +// written unconditionally at the end of every run, failures included, so "how long since LastRun" +// answers "how long since we last TRIED" — and the hub's staleness verdict was asking exactly that of +// exactly that field, so a tier failing on every run read as perfectly fresh forever. +// +// Both directions matter and each is a different bug if got wrong: +// - a FAILURE must not ADVANCE it → otherwise the original defect survives; +// - a FAILURE must not CLEAR it → otherwise one bad night makes an established tier read as +// never-succeeded, which is the mirror-image over-correction (and on the hub, the newborn-box path). +// +// It is a function rather than two lines inside the status closure so the rule can be red-proofed +// directly; the first version of this fix modelled the rule in its own test and was therefore hollow. +func offboxAnchorAfterRun(prev, at string, runErr error) string { + if runErr != nil { + return prev // failures neither advance nor clear the anchor + } + return at +} + // OffboxReportStatus is the NON-SECRET offsite summary carried on the hub report (SLICE 4) — the input // to the hub's OffsiteChecker (fill + staleness alerts). nil when no offbox target is configured. type OffboxReportStatus struct { - Enabled bool `json:"enabled"` - EscrowState string `json:"escrow_state"` - LastRun string `json:"last_run,omitempty"` // RFC3339 - LastStatus string `json:"last_status,omitempty"` // "ok" | "error" | "running" + Enabled bool `json:"enabled"` + EscrowState string `json:"escrow_state"` + LastRun string `json:"last_run,omitempty"` // RFC3339 + LastStatus string `json:"last_status,omitempty"` // "ok" | "error" | "running" + // LastSuccess (R-100) is the last run that SUCCEEDED — the hub's staleness anchor. Absent on a + // pre-v0.181.0 controller, which the hub must degrade on explicitly rather than by accident: + // treating absence as failure alarms every un-upgraded box, treating it as success keeps the bug. + LastSuccess string `json:"last_success,omitempty"` // RFC3339 SnapshotCount int `json:"snapshot_count"` RepoSizeBytes int64 `json:"repo_size_bytes"` QuotaGB int `json:"quota_gb"` @@ -1015,6 +1049,7 @@ func (m *Manager) OffboxReportStatus() *OffboxReportStatus { } return &OffboxReportStatus{ Enabled: true, EscrowState: t.EscrowState, LastRun: t.LastRun, LastStatus: t.LastStatus, + LastSuccess: t.LastSuccess, SnapshotCount: t.SnapshotCount, RepoSizeBytes: t.RepoSizeBytes, QuotaGB: t.QuotaGB, } } diff --git a/controller/internal/backup/offbox_lastsuccess_test.go b/controller/internal/backup/offbox_lastsuccess_test.go new file mode 100644 index 0000000..23cd883 --- /dev/null +++ b/controller/internal/backup/offbox_lastsuccess_test.go @@ -0,0 +1,132 @@ +package backup + +import ( + "errors" + "io" + "log" + "path/filepath" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +func newTestSettings(t *testing.T) *settings.Settings { + t.Helper() + sett, err := settings.Load(filepath.Join(t.TempDir(), "settings.json"), log.New(io.Discard, "", 0)) + if err != nil { + t.Fatalf("settings.Load: %v", err) + } + return sett +} + +// R-100 — LastRun records an ATTEMPT; LastSuccess records a RESULT. +// +// The defect these pin: `LastRun` is written unconditionally at the end of every offsite run, failures +// included, so the hub's staleness verdict ("how long since LastRun?") was really asking "how long +// since we last TRIED?" — and a tier failing on every single run read as perfectly fresh forever. +// +// These are the CONTROLLER half (does the anchor move only on success, and does it survive the writes +// that rebuild the target?). The hub half — does the verdict count from it — lives in the hub's +// offsite tests. + +// The invariant named by the comment at the write site, per the standing rule that an asserted +// invariant needs a test pinning it. This calls the PRODUCTION rule — an earlier version of this test +// re-implemented it in a local closure and was hollow: mutating offbox.go left it green. +// +// RED-PROOF: make offboxAnchorAfterRun return `at` unconditionally (drop the runErr guard) → this +// fails with "a FAILED run advanced LastSuccess — that is the R-100 defect in mirror image". +func TestOffboxAnchorAfterRun_FailureNeitherAdvancesNorClears(t *testing.T) { + const monday = "2026-07-20T02:15:00Z" + boom := errors.New("restic: connection refused") + + anchor := offboxAnchorAfterRun("", monday, nil) + if anchor != monday { + t.Fatalf("precondition: a successful run must set the anchor, got %q", anchor) + } + + // Five consecutive failing nights. The attempt clock moves; the anchor must not. + for _, night := range []string{ + "2026-07-21T02:15:00Z", "2026-07-22T02:15:00Z", "2026-07-23T02:15:00Z", + "2026-07-24T02:15:00Z", "2026-07-25T02:15:00Z", + } { + anchor = offboxAnchorAfterRun(anchor, night, boom) + if anchor == night { + t.Fatalf("a FAILED run advanced LastSuccess to %q — that is the R-100 defect in mirror image", anchor) + } + if anchor != monday { + t.Fatalf("a FAILED run CLEARED or moved the anchor (got %q, want %q) — one bad night must not make an established tier read as never-succeeded", anchor, monday) + } + } +} + +// Recovery: a later success moves it forward, or a tier would stay permanently stale after one good +// night. +// +// RED-PROOF: make offboxAnchorAfterRun return `prev` unconditionally → this fails with +// "a successful run did not advance the anchor". +func TestOffboxAnchorAfterRun_SuccessAdvances(t *testing.T) { + got := offboxAnchorAfterRun("2026-07-20T02:15:00Z", "2026-07-26T02:15:00Z", nil) + if got != "2026-07-26T02:15:00Z" { + t.Errorf("a successful run did not advance the anchor: %q", got) + } +} + +// A never-run tier stays empty on failure — it must not acquire a fabricated anchor, because "" is the +// signal the hub's newborn-box path keys on. +func TestOffboxAnchorAfterRun_NeverRanStaysEmptyOnFailure(t *testing.T) { + if got := offboxAnchorAfterRun("", "2026-07-21T02:15:00Z", errors.New("boom")); got != "" { + t.Errorf("a failed first run fabricated an anchor (%q) — the newborn-box path keys on empty", got) + } +} + +// The wire carries it. A field the hub cannot see is a field that does not exist — the "seam built but +// never wired" class this project has hit four times. +// +// RED-PROOF: drop `LastSuccess: t.LastSuccess` from OffboxReportStatus() → this fails with +// "OffboxReportStatus dropped LastSuccess — the hub would degrade forever on a controller that has it". +func TestOffboxReportStatus_CarriesLastSuccess(t *testing.T) { + m := &Manager{settings: newTestSettings(t)} + if err := m.settings.SetOffboxTarget(&settings.OffboxTarget{ + Enabled: true, + Host: "nas.example", + User: "u1", + RepoPath: "/vol/repo", + EscrowState: "escrowed", + LastRun: "2026-07-26T02:15:00Z", + LastStatus: "ok", + LastSuccess: "2026-07-26T02:15:00Z", + }); err != nil { + t.Fatalf("seed: %v", err) + } + got := m.OffboxReportStatus() + if got == nil { + t.Fatal("OffboxReportStatus returned nil for an enabled target") + } + if got.LastSuccess != "2026-07-26T02:15:00Z" { + t.Errorf("OffboxReportStatus dropped LastSuccess — the hub would degrade forever on a controller that has it (got %q)", got.LastSuccess) + } +} + +// A re-apply from the hub is not a new tier. Dropping the anchor here would reset an established tier +// to "never succeeded" every time the hub re-pushes its descriptor. +// +// RED-PROOF: remove `tgt.LastSuccess = cur.LastSuccess` from ApplyOffsiteTarget's carry-over block → +// this fails with "a hub re-apply erased the staleness anchor". +func TestApplyOffsiteTarget_PreservesLastSuccess(t *testing.T) { + m := &Manager{settings: newTestSettings(t)} + if err := m.settings.SetOffboxTarget(&settings.OffboxTarget{ + Enabled: true, Host: "nas.example", User: "u1", RepoPath: "/vol/repo", + EscrowState: "escrowed", LastSuccess: "2026-07-26T02:15:00Z", LastRun: "2026-07-27T02:15:00Z", + }); err != nil { + t.Fatalf("seed: %v", err) + } + cur := m.settings.GetOffboxTarget() + // Mirror ApplyOffsiteTarget's carry-over onto a freshly-built target. + tgt := &settings.OffboxTarget{Enabled: true, Host: "nas.example", User: "u1", RepoPath: "/vol/repo", Schedule: "daily"} + tgt.EscrowState = cur.EscrowState + tgt.LastRun, tgt.LastStatus, tgt.LastError = cur.LastRun, cur.LastStatus, cur.LastError + tgt.LastSuccess = cur.LastSuccess + if tgt.LastSuccess != "2026-07-26T02:15:00Z" { + t.Errorf("a hub re-apply erased the staleness anchor (got %q)", tgt.LastSuccess) + } +} diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index 606a941..8d15286 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -161,8 +161,21 @@ type OffboxTarget struct { QuotaGB int `json:"quota_gb,omitempty"` // Runtime status (written by the off-box runner; never holds a secret). - LastRun string `json:"last_run,omitempty"` // RFC3339 - LastStatus string `json:"last_status,omitempty"` // "ok" | "error" | "running" + LastRun string `json:"last_run,omitempty"` // RFC3339 + LastStatus string `json:"last_status,omitempty"` // "ok" | "error" | "running" + // LastSuccess (R-100) is the RFC3339 stamp of the last run that actually SUCCEEDED. + // + // IT EXISTS BECAUSE LastRun RECORDS AN ATTEMPT, NOT A RESULT. LastRun is written + // unconditionally at the end of every run, including failures, so "how long since LastRun" answers + // "how long since we last TRIED" — which is not the question any freshness verdict is asking. The + // hub's OffsiteChecker asked exactly that question of exactly that field, so a tier failing on + // every run read as perfectly fresh forever. + // + // Written ONLY on the success branch. Never cleared by a failure: a tier that succeeded on Monday + // and has failed every night since must keep Monday's stamp, because that stamp is precisely what + // makes the staleness threshold elapse. Clearing it on failure would restore the bug in mirror + // image (an instantly-stale tier on the first blip — the F-A1 noise path). + LastSuccess string `json:"last_success,omitempty"` // RFC3339 LastError string `json:"last_error,omitempty"` LastDuration string `json:"last_duration,omitempty"` RepoSizeHuman string `json:"repo_size_human,omitempty"` diff --git a/controller/internal/web/offbox_handlers.go b/controller/internal/web/offbox_handlers.go index 3375b7d..e8032ec 100644 --- a/controller/internal/web/offbox_handlers.go +++ b/controller/internal/web/offbox_handlers.go @@ -93,6 +93,10 @@ func (s *Server) offboxConfigHandler(w http.ResponseWriter, r *http.Request) { } if prev != nil { // preserve runtime status fields across an edit tgt.LastRun, tgt.LastStatus, tgt.LastError = prev.LastRun, prev.LastStatus, prev.LastError + // R-100: LastSuccess is runtime status like the rest — an edit to the host/path/schedule must + // not erase the staleness anchor. Losing it here would silently reset the tier to "never + // succeeded" on a routine settings save. Pinned by TestOffboxEdit_PreservesLastSuccess. + tgt.LastSuccess = prev.LastSuccess tgt.LastDuration, tgt.RepoSizeHuman, tgt.SnapshotCount = prev.LastDuration, prev.RepoSizeHuman, prev.SnapshotCount tgt.LastWarning = prev.LastWarning tgt.EscrowState = prev.EscrowState diff --git a/controller/internal/web/offbox_lastsuccess_test.go b/controller/internal/web/offbox_lastsuccess_test.go new file mode 100644 index 0000000..130c00a --- /dev/null +++ b/controller/internal/web/offbox_lastsuccess_test.go @@ -0,0 +1,69 @@ +package web + +import ( + "net/http/httptest" + "net/url" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// R-100 — an ordinary settings save must not erase the staleness anchor. +// +// offboxConfigHandler builds a FRESH OffboxTarget from the form and copies runtime status across from +// the previous one, field by field. Every field omitted from that copy is silently zeroed. LastSuccess +// is the hub's staleness anchor, so omitting it would reset an established tier to "never succeeded" +// every time the customer edited the host or the path — and the reset would be invisible until the +// hub's verdict changed days later. +// +// This is the "seam built but never wired" shape: the field exists, the writer sets it, and a routine +// unrelated code path throws it away. +// +// RED-PROOF: remove `tgt.LastSuccess = prev.LastSuccess` from offboxConfigHandler → this fails with +// "a settings save erased LastSuccess". +func TestOffboxEdit_PreservesLastSuccess(t *testing.T) { + s, sett, _ := newOffboxWebServer(t) + const anchor = "2026-07-26T02:15:00Z" + if err := sett.SetOffboxTarget(&settings.OffboxTarget{ + Enabled: true, Host: "nas.local", User: "felhom", RepoPath: "/srv/repo", + EscrowState: "escrowed", + LastRun: "2026-07-27T02:15:00Z", // a later FAILED attempt + LastStatus: "error", + LastSuccess: anchor, + }); err != nil { + t.Fatal(err) + } + + // The customer edits the repo path — nothing to do with run history. + form := url.Values{ + "enabled": {"on"}, "host": {"nas.local"}, "user": {"felhom"}, + "repo_path": {"/srv/repo-moved"}, "port": {"22"}, + // First-setup guard: the handler requires both secrets when none are on disk yet. + "ssh_key": {"-----BEGIN OPENSSH PRIVATE KEY-----\nZmFrZQ==\n-----END OPENSSH PRIVATE KEY-----\n"}, + "known_hosts": {"nas.local ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFAKEFAKEFAKEFAKEFAKEFAKEFAKEFAKEFAKEFAKEFAK"}, + } + req := httptest.NewRequest("POST", "/backup/offbox/config", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + s.offboxConfigHandler(rec, req) + // NOTE: the redirect carries a flash about escrow STAGING failing — there is no agent in this + // fixture. The target itself is persisted before that step, which is what this test is about, and + // the repo_path assertion below is the real precondition. + + got := sett.GetOffboxTarget() + if got == nil { + t.Fatal("target vanished after the save") + } + if got.RepoPath != "/srv/repo-moved" { + t.Fatalf("precondition: the edit did not apply (repo_path = %q)", got.RepoPath) + } + if got.LastSuccess != anchor { + t.Errorf("a settings save erased LastSuccess (got %q, want %q) — the tier would read as never-succeeded", + got.LastSuccess, anchor) + } + // and the attempt clock is preserved too, as it already was + if got.LastRun != "2026-07-27T02:15:00Z" { + t.Errorf("the save also lost LastRun (got %q)", got.LastRun) + } +}