fcef8e069c
gates / gates (push) Successful in 12s
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.
225 lines
9.2 KiB
Go
225 lines
9.2 KiB
Go
package backup
|
|
|
|
import (
|
|
"go/ast"
|
|
"go/parser"
|
|
"go/token"
|
|
"io/fs"
|
|
"path/filepath"
|
|
"sort"
|
|
"strings"
|
|
"testing"
|
|
)
|
|
|
|
// R-408 — THE INVARIANT IS PINNED BY A WALK, NOT ASSERTED IN A COMMENT.
|
|
//
|
|
// THE DEFECT THIS EXISTS FOR IS NOT THE MISSING ACQUIRE. `offbox_integrity.go`'s header has said
|
|
// *"Every off-site operation takes `acquireRunning` for exactly that reason"* since v0.227.0, nothing
|
|
// ever checked it, and it was **false for months** — `RestoreOffboxScratch` and
|
|
// `OffboxRestorePrepareFull` both issued restic commands without it. `resticStep`'s licence to run
|
|
// `unlock --remove-all` rests entirely on that sentence being true, so a false sentence there is a
|
|
// licence to delete a live operation's lock. Measured on demo-hp 2026-08-31, R-411.
|
|
//
|
|
// That is the ninth instance of this project's most-repeated class: a comment stating a guarantee the
|
|
// code does not provide. The workspace rule is *"a comment asserting an invariant needs a test pinning
|
|
// it, or it is a wish"*. **This is that test.**
|
|
//
|
|
// WHAT IT ASSERTS. Every function in `internal/backup` that reaches the off-site repository — by
|
|
// calling `resticStep`, `resticBackupStep` or the raw exec seam `m.runner()` — must EITHER call
|
|
// `acquireRunning` itself, OR be reachable only from a function that does, OR be registered below by
|
|
// name with the reason it is exempt.
|
|
//
|
|
// IT IS AN AST WALK AND NOT `strings.Contains`, deliberately: a commented-out call still contains the
|
|
// string. That is exactly how an earlier version of a test in this repo passed its own red-proof.
|
|
|
|
// offsiteExempt registers the functions that reach restic WITHOUT the flag, each with the reason.
|
|
// Adding a line here is a deliberate act and should be argued in the commit that adds it.
|
|
var offsiteExempt = map[string]string{
|
|
// R-87's proof restore. It is READ-ONLY BY CONSTRUCTION: `--no-lock`, no `unlockStale`, and it goes
|
|
// through `m.runner()` rather than `resticStep`, so the `unlock --remove-all` escalation is
|
|
// unreachable rather than merely unlikely. It cannot remove anyone's lock and it cannot take one.
|
|
// Its CALLER, `ProveOffboxUnit`, does take the flag — this is the inner helper.
|
|
"restoreUnitReadOnly": "R-87: --no-lock, no unlockStale, via m.runner() — cannot take or remove a lock; its caller ProveOffboxUnit holds the flag",
|
|
|
|
// The lock-hygiene helpers themselves. They are only ever called from inside a function that
|
|
// already holds the flag; flagging them would deadlock, since acquireRunning is not reentrant.
|
|
"unlockStale": "lock hygiene, called only from inside a flag-holding caller; acquireRunning is not reentrant",
|
|
"resticStep": "the shared step runner — its own doc comment records that every CALLER holds the flag, which is what this test pins",
|
|
|
|
// Probes and readers that take no lock. `restic snapshots` and `restic list` were measured on
|
|
// demo-hp 2026-08-31 NOT to lock (6 back-to-back invocations, sampler read locks=0 throughout).
|
|
"offboxLatestSnapshot": "restic snapshots — measured 2026-08-31 not to take a lock; always called from a flag-holding caller anyway",
|
|
"ensureOffboxRepo": "restic cat config / init probe, called from inside flag-holding callers only",
|
|
"offboxInventory": "restic snapshots --json, a read; no lock taken",
|
|
// Read-only listing behind two web pages (the recovery page and the restore list). It issues only
|
|
// `restic snapshots --json`, and `snapshots` was MEASURED on demo-hp 2026-08-31 not to take a lock
|
|
// — six back-to-back invocations, the sampler read locks=0 throughout. Flagging it would make
|
|
// browsing a page refuse while a backup runs, for no safety gain: it can neither take a lock nor
|
|
// remove one.
|
|
"OffsiteInventoryList": "restic snapshots --json only; snapshots measured 2026-08-31 not to lock, and it never routes through resticStep",
|
|
}
|
|
|
|
// offsiteReachers are the calls that mean "this function talks to the off-site repository".
|
|
var offsiteReachers = map[string]bool{
|
|
"resticStep": true, "resticBackupStep": true, "runner": true,
|
|
}
|
|
|
|
func r408WalkBackupPackage(t *testing.T) (map[string]*ast.FuncDecl, *token.FileSet) {
|
|
t.Helper()
|
|
root, err := filepath.Abs(".")
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
fset := token.NewFileSet()
|
|
fns := map[string]*ast.FuncDecl{}
|
|
err = filepath.WalkDir(root, func(path string, d fs.DirEntry, werr error) error {
|
|
if werr != nil {
|
|
return werr
|
|
}
|
|
if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
|
|
return nil
|
|
}
|
|
f, perr := parser.ParseFile(fset, path, nil, 0) // comments DROPPED on purpose
|
|
if perr != nil {
|
|
t.Fatalf("parse %s: %v — the R-408 invariant is now unasserted", path, perr)
|
|
}
|
|
for _, decl := range f.Decls {
|
|
if fd, ok := decl.(*ast.FuncDecl); ok && fd.Body != nil {
|
|
fns[fd.Name.Name] = fd
|
|
}
|
|
}
|
|
return nil
|
|
})
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
return fns, fset
|
|
}
|
|
|
|
func r408Calls(fd *ast.FuncDecl) map[string]bool {
|
|
out := map[string]bool{}
|
|
ast.Inspect(fd.Body, func(n ast.Node) bool {
|
|
call, ok := n.(*ast.CallExpr)
|
|
if !ok {
|
|
return true
|
|
}
|
|
switch fn := call.Fun.(type) {
|
|
case *ast.Ident:
|
|
out[fn.Name] = true
|
|
case *ast.SelectorExpr:
|
|
out[fn.Sel.Name] = true
|
|
// `m.runner()(ctx, env, args...)` — the seam is invoked through the value it returns, so
|
|
// the outer CallExpr's Fun is itself a CallExpr. Catch that shape explicitly.
|
|
}
|
|
if inner, ok := call.Fun.(*ast.CallExpr); ok {
|
|
if sel, ok := inner.Fun.(*ast.SelectorExpr); ok {
|
|
out[sel.Sel.Name] = true
|
|
}
|
|
}
|
|
return true
|
|
})
|
|
return out
|
|
}
|
|
|
|
// TestR408_EveryOffsiteEntryPointTakesTheFlagOrIsRegistered — B1.
|
|
//
|
|
// RED-PROOF (run 2026-09-01, recorded in REPORT.md): removing the `acquireRunning` from
|
|
// `RestoreOffboxScratch` makes this fail naming that function; adding an unregistered fake entry point
|
|
// that calls `m.resticStep(...)` makes it fail naming the fake.
|
|
func TestR408_EveryOffsiteEntryPointTakesTheFlagOrIsRegistered(t *testing.T) {
|
|
fns, _ := r408WalkBackupPackage(t)
|
|
|
|
calls := map[string]map[string]bool{}
|
|
for name, fd := range fns {
|
|
calls[name] = r408Calls(fd)
|
|
}
|
|
|
|
// A function is COVERED if it acquires the flag itself, or every path to it is through a function
|
|
// that does. Fixed-point: start from the self-acquirers and propagate to their callees.
|
|
covered := map[string]bool{}
|
|
for name, c := range calls {
|
|
if c["acquireRunning"] {
|
|
covered[name] = true
|
|
}
|
|
}
|
|
if len(covered) == 0 {
|
|
t.Fatal("no function in internal/backup calls acquireRunning — the walk is looking in the wrong place, and a green here would be meaningless")
|
|
}
|
|
for i := 0; i < 12; i++ { // depth cap; the call graph here is shallow
|
|
grew := false
|
|
for name := range covered {
|
|
for callee := range calls[name] {
|
|
if _, ours := fns[callee]; ours && !covered[callee] {
|
|
covered[callee] = true
|
|
grew = true
|
|
}
|
|
}
|
|
}
|
|
if !grew {
|
|
break
|
|
}
|
|
}
|
|
|
|
var offenders []string
|
|
for name, c := range calls {
|
|
reaches := false
|
|
for r := range offsiteReachers {
|
|
if c[r] {
|
|
reaches = true
|
|
break
|
|
}
|
|
}
|
|
if !reaches || covered[name] || offsiteExempt[name] != "" {
|
|
continue
|
|
}
|
|
offenders = append(offenders, name)
|
|
}
|
|
sort.Strings(offenders)
|
|
|
|
if len(offenders) > 0 {
|
|
t.Fatalf("these functions reach the off-site repository without the single-writer flag and are not registered exempt: %v\n\n"+
|
|
"`resticStep` escalates to `unlock --remove-all` on a lock error, and its licence to do that is\n"+
|
|
"that every caller holds the flag. R-411 measured what happens when one does not: a live\n"+
|
|
"customer restore's lock was deleted and logged as a crash that never happened.\n\n"+
|
|
"Take acquireRunning, or add the function to offsiteExempt WITH the reason it cannot collide.", offenders)
|
|
}
|
|
}
|
|
|
|
// TestR408_TheProofsReadOnlyPathIsARegisteredException — B2.
|
|
//
|
|
// The exemption must stay HONEST: `restoreUnitReadOnly` is exempt only because it is read-only. If it
|
|
// ever gains `unlockStale`, or routes through `resticStep`, or loses `--no-lock`, the exemption is a
|
|
// lie and this fails.
|
|
func TestR408_TheProofsReadOnlyPathIsARegisteredException(t *testing.T) {
|
|
if offsiteExempt["restoreUnitReadOnly"] == "" {
|
|
t.Fatal("restoreUnitReadOnly must be a REGISTERED exception, with its reason, not silently absent")
|
|
}
|
|
fns, _ := r408WalkBackupPackage(t)
|
|
fd := fns["restoreUnitReadOnly"]
|
|
if fd == nil {
|
|
t.Fatal("restoreUnitReadOnly is gone — the exemption now covers nothing and must be removed")
|
|
}
|
|
c := r408Calls(fd)
|
|
if c["resticStep"] || c["resticBackupStep"] {
|
|
t.Fatal("restoreUnitReadOnly now routes through resticStep — the unlock --remove-all escalation is reachable and the exemption is no longer true")
|
|
}
|
|
if c["unlockStale"] {
|
|
t.Fatal("restoreUnitReadOnly now calls unlockStale — it issues a delete verb and the exemption is no longer true")
|
|
}
|
|
// And it must still pass --no-lock.
|
|
var sawNoLock bool
|
|
ast.Inspect(fd.Body, func(n ast.Node) bool {
|
|
if lit, ok := n.(*ast.BasicLit); ok && strings.Trim(lit.Value, `"`) == "--no-lock" {
|
|
sawNoLock = true
|
|
}
|
|
return true
|
|
})
|
|
if !sawNoLock {
|
|
t.Fatal("restoreUnitReadOnly no longer passes --no-lock — it can now take a lock and the exemption is no longer true")
|
|
}
|
|
// Its caller must hold the flag, or the exemption rests on nothing.
|
|
if !r408Calls(fns["ProveOffboxUnit"])["acquireRunning"] {
|
|
t.Fatal("ProveOffboxUnit no longer takes the flag — restoreUnitReadOnly's exemption depends on its caller holding it")
|
|
}
|
|
}
|