Files
felhom-controller/controller/internal/backup/r411_lock_flag_test.go
T
admin fcef8e069c
gates / gates (push) Successful in 12s
one writer at a time, and a check that can run (R-411, R-408, R-407, R-414, R-412a)
THE WALK FOUND THREE MORE ENTRY POINTS THAN THE REPORT DID. R-411 named one missing
acquireRunning. Fixing it and then pinning the invariant with an AST walk surfaced FOUR in
total, all of which issued restic commands with no flag:

  RestoreOffboxScratch      - the reported one
  OffboxRestorePrepareFull  - the SECOND request in the customer's own two-step full-restore
                              flow, and the one that actually shells `restic stats`. The UI
                              reaches it FIRST, so flagging only the restore would have left
                              the collision reachable by the ordinary path.
  RestoreSharesScratch      - R-411's exact shape on the shares tier: unlockStale + resticStep,
                              a live web caller, and its sibling PlaceSharesRestore has always
                              taken the flag.
  RestoreOffbox             - no production caller today, but the same dangerous pattern.
                              Flagged rather than left for a future caller to inherit.

OffsiteInventoryList is REGISTERED EXEMPT with its reason: it issues only `restic snapshots
--json`, measured on demo-hp 2026-08-31 not to take a lock, and flagging it would make
browsing a page refuse during a backup for no safety gain.

THE REAL DELIVERABLE IS THE WALK, not the acquire. offbox_integrity.go:28 asserted "Every
off-site operation takes acquireRunning" since v0.227.0, nothing checked it, and it was false
for months - the ninth instance of this project's most-repeated class. The walk is an AST
pass, not strings.Contains, because a commented-out call still contains the string.
Red-proofed twice: removing the acquire fails it naming RestoreOffboxScratch; an
unregistered fake entry point fails it naming the fake.

R-407: "It NEVER writes to the repository" corrected in place, not deleted (R-360's rule).
`check` takes a lock - and so does `restic stats`, which is the fact nobody had and the one
that made R-411 possible. Both recorded where the next reader will meet them.

R-414: the proof could not run at all on a box with no registered drive. Part 2.1's
determination came out as neither "missed" nor "deliberate": R-356's own test comments say
the scratch resolver "still resolves ... only the DESTINATION moves", so it was OUT OF SCOPE,
and it was never ruled out on state-only grounds - the one comment about a systemDataPath
fallback belonged to PlaceOffsiteRestore, concerned bulk USERDATA, and R-356 overruled even
that. So 07 section 6.3's rule applies and now has a fourth consumer.

The fallback is SCOPED, because the two callers ask different questions and one predicate
answering both is the R-356 defect itself: a UNIT-ONLY restore may fall back to the system
data path (07 section 7 records as FACT that a driveless app's unit already lives there
indefinitely, and that the same-device placement is intended); a FULL restore keeps today's
refusal, because it pulls bulk userdata onto a state-only tier.

And the silence ends either way: a proof that cannot start now records ProofResultCannotRun
rather than an Err, so last_proof_result is never ABSENT - absent already means "controller
too old", and a second meaning on the same field is the StatsKnown trap one level up. It is
recorded WITHOUT advancing per-snapshot due-ness, so the app stays retryable once a drive is
registered.

R-412 leg 1: a per-app push whose unit carried no dump and no tar now says so, at WARN.
Wording only - no guard, and the capture is untouched (08 section 8.2). Leg 2 stays OPEN.

16 new tests, 1689 -> 1705. Full suite 28 packages rc=0, all 13 controller gates OK.
Red-proofs run and reverted byte-identical for A3/B1 (twice), C1 and D1.
2026-09-01 10:19:36 +02:00

180 lines
6.4 KiB
Go

package backup
import (
"context"
"os"
"strings"
"sync"
"testing"
"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
)
// R-411 / R-408 — the single-writer flag on the customer restore paths.
//
// These drive the real functions through the restic exec seam (`SetOffboxRunner`), which REUSE.md
// names as the way to see the argv — replacing `resticStep` would hide the `unlock --remove-all`
// escalation from exactly the assertion that must see it.
// lockHarness records every restic argv and lets a test hold the flag.
type lockHarness struct {
m *Manager
mu sync.Mutex
argv [][]string
}
func newLockHarness(t *testing.T, stacks ...string) *lockHarness {
t.Helper()
m, sett := newOffboxManager(t)
drive := t.TempDir()
if err := sett.AddStoragePath(settings.StoragePath{Path: drive, Label: "data", Schedulable: true}); err != nil {
t.Fatal(err)
}
deployed := map[string]bool{}
for _, s := range stacks {
deployed[s] = true
}
m.SetStackProvider(&offbox3aProvider{
hdd: map[string]string{}, binds: map[string][]ClassifiedBind{},
has: map[string]bool{}, deployed: deployed,
})
h := &lockHarness{m: m}
m.SetOffboxLatestSnapshotFn(func(_ context.Context, stack string) (string, []string, error) {
return "snap-" + stack, []string{"/mnt/sys_drive/felhom-data/backups/primary/" + stack}, nil
})
m.SetOffboxRunner(func(_ context.Context, _ []string, args ...string) ([]byte, error) {
h.mu.Lock()
h.argv = append(h.argv, append([]string{}, args...))
h.mu.Unlock()
return []byte("{}"), nil
})
m.SetOffboxFreeFn(func(string) int64 { return 100 << 30 })
return h
}
func (h *lockHarness) allArgs() [][]string {
h.mu.Lock()
defer h.mu.Unlock()
return h.argv
}
// TestR411_ScratchRestoreTakesTheSingleWriterFlag — A1.
func TestR411_ScratchRestoreTakesTheSingleWriterFlag(t *testing.T) {
h := newLockHarness(t, "kimai")
// Hold the flag as a sibling operation would.
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatalf("could not take the flag: %v", err)
}
err := h.m.RestoreOffboxScratch(context.Background(), "kimai", false)
if err == nil {
t.Fatal("a scratch restore must REFUSE while the single-writer flag is held — before this fix it ran anyway, which is R-411")
}
if len(h.allArgs()) != 0 {
t.Fatalf("a refused restore must invoke restic ZERO times; got %v", h.allArgs())
}
}
// TestR411_PrepareFullAlsoTakesTheFlag — the SECOND entry point, found while fixing the first.
// The customer's real UI flow reaches this one first, and it is the one that shells `restic stats`.
func TestR411_PrepareFullAlsoTakesTheFlag(t *testing.T) {
h := newLockHarness(t, "kimai")
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatal(err)
}
if _, err := h.m.OffboxRestorePrepareFull(context.Background(), "kimai"); err == nil {
t.Fatal("the full-restore PREPARE must refuse while the flag is held — it shells `restic stats`, which takes a repository lock")
}
if len(h.allArgs()) != 0 {
t.Fatalf("a refused prepare must invoke restic ZERO times; got %v", h.allArgs())
}
}
// TestR411_SharesScratchRestoreAlsoTakesTheFlag — the third, found by the R-408 walk.
func TestR411_SharesScratchRestoreAlsoTakesTheFlag(t *testing.T) {
h := newLockHarness(t)
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatal(err)
}
if err := h.m.RestoreSharesScratch(context.Background()); err == nil {
t.Fatal("the shares scratch restore must refuse while the flag is held — it runs unlockStale + resticStep, R-411's exact shape")
}
if len(h.allArgs()) != 0 {
t.Fatalf("a refused shares restore must invoke restic ZERO times; got %v", h.allArgs())
}
}
// TestR411_NoUnlockRemoveAllInAnyArgv — A3, the non-effect that matters.
//
// RED-PROOF (run 2026-09-01): removing the acquire from `RestoreOffboxScratch` lets the restore run
// while the flag is held, so an integrity check could collide with it — the state this asserts is
// impossible. The direct form of the red-proof is `TestR408_…`, which names the function.
func TestR411_NoUnlockRemoveAllInAnyArgv(t *testing.T) {
h := newLockHarness(t, "kimai")
if err := h.m.RestoreOffboxScratch(context.Background(), "kimai", false); err != nil {
t.Fatalf("an idle-box restore must succeed: %v", err)
}
for _, args := range h.allArgs() {
joined := strings.Join(args, " ")
if strings.Contains(joined, "--remove-all") {
t.Fatalf("the escalation fired during an ordinary restore: %s", joined)
}
}
if len(h.allArgs()) == 0 {
t.Fatal("no restic invocation at all — the assertion above ran over nothing")
}
}
// TestR411_RestoreStillWorksWhenIdle — Scenario C. The flag must not make the common case refuse.
func TestR411_RestoreStillWorksWhenIdle(t *testing.T) {
h := newLockHarness(t, "kimai")
if err := h.m.RestoreOffboxScratch(context.Background(), "kimai", false); err != nil {
t.Fatalf("a restore on an idle box must still work: %v", err)
}
var sawRestore bool
for _, args := range h.allArgs() {
if containsArg(args, "restore") {
sawRestore = true
}
}
if !sawRestore {
t.Fatal("no restore was issued")
}
}
// TestR411_FlagIsReleasedOnEveryPath — A5. A flag that leaks would wedge every nightly job.
func TestR411_FlagIsReleasedOnEveryPath(t *testing.T) {
t.Run("success", func(t *testing.T) {
h := newLockHarness(t, "kimai")
if err := h.m.RestoreOffboxScratch(context.Background(), "kimai", false); err != nil {
t.Fatal(err)
}
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatal("the flag was NOT released after a successful restore")
}
})
t.Run("restic error", func(t *testing.T) {
h := newLockHarness(t, "kimai")
h.m.SetOffboxRunner(func(_ context.Context, _ []string, _ ...string) ([]byte, error) {
return []byte("boom"), os.ErrPermission
})
_ = h.m.RestoreOffboxScratch(context.Background(), "kimai", false)
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatal("the flag was NOT released after a failed restore")
}
})
t.Run("early refusal", func(t *testing.T) {
h := newLockHarness(t, "kimai")
_ = h.m.RestoreOffboxScratch(context.Background(), "no such stack!!", false)
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatal("the flag was NOT released after an early refusal")
}
})
t.Run("prepare", func(t *testing.T) {
h := newLockHarness(t, "kimai")
_, _ = h.m.OffboxRestorePrepareFull(context.Background(), "kimai")
if err := h.m.AcquireRunningForTest(); err != nil {
t.Fatal("the flag was NOT released after the prepare step")
}
})
}