one writer at a time, and a check that can run (R-411, R-408, R-407, R-414, R-412a)
gates / gates (push) Successful in 12s
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.
This commit is contained in:
@@ -177,7 +177,9 @@ func (m *Manager) offboxSnapshotSize(ctx context.Context, id string) (int64, err
|
||||
func (m *Manager) offboxRestoreScratchDir(stack string) (scratch, nsRoot string, err error) {
|
||||
// offsiteRestoreRootFor is THE place `backups/offsite-restore` is spelled (offbox_verify_copies.go)
|
||||
// — the listing/delete surface must resolve byte-identical paths to the ones written here.
|
||||
return m.offboxScratchDirIn(stack, m.offsiteRestoreRootFor)
|
||||
// unitOnly=false: the CUSTOMER's scratch can hold a full restore (bulk userdata), so it must NOT
|
||||
// fall back to the state-only system disk — see offboxScratchDirIn.
|
||||
return m.offboxScratchDirIn(stack, m.offsiteRestoreRootFor, false)
|
||||
}
|
||||
|
||||
// offboxProofScratchDir is the R-87 nightly proof's scratch, resolved by the SAME drive-preference
|
||||
@@ -190,14 +192,49 @@ func (m *Manager) offboxRestoreScratchDir(stack string) (scratch, nsRoot string,
|
||||
// copy invisible to `DeleteOffsiteRestoreCopy`, the copy listing and `OffboxFullScratchReady`, so it
|
||||
// can never be offered for placement into a live app.
|
||||
func (m *Manager) offboxProofScratchDir(stack string) (scratch, nsRoot string, err error) {
|
||||
return m.offboxScratchDirIn(stack, m.offsiteProofRootFor)
|
||||
// unitOnly=true: the proof restores ONE recovery unit (`--include <unit>`) and deletes it. For a
|
||||
// driveless app that unit already lives permanently on the system data path, so a scratch there is
|
||||
// at most a second copy of something already present — see offboxScratchDirIn.
|
||||
return m.offboxScratchDirIn(stack, m.offsiteProofRootFor, true)
|
||||
}
|
||||
|
||||
// offboxScratchDirIn holds the drive-preference rules once. `rootFor` chooses WHICH root under the
|
||||
// namespace the scratch lands in; everything else — the network-storage refusal, the ordering, the
|
||||
// R-252 wording — is shared, so the proof path can never drift from the customer path on the parts
|
||||
// that must not differ.
|
||||
func (m *Manager) offboxScratchDirIn(stack string, rootFor func(string) string) (scratch, nsRoot string, err error) {
|
||||
// R-414 — THE SYSTEM-DATA FALLBACK, AND WHY IT IS SCOPED BY WHAT IS BEING RESTORED.
|
||||
//
|
||||
// THE GAP. `demo-felhom` has ZERO registered storage paths, so steps (1)-(3) all miss and this
|
||||
// refused. The nightly proof therefore could not run AT ALL on that box — every night, with only a
|
||||
// WARN — and because its error path reaches no verdict, `last_proof_result` stayed ABSENT, which is
|
||||
// also what a controller too old to have the feature sends. The hub could not tell them apart.
|
||||
//
|
||||
// WAS IT MISSED OR DELIBERATE? Established from R-356's own commit (`08eb1a6`, 2026-08-22), whose test
|
||||
// comments say the scratch resolver *"still resolves to the registered storage path … only the
|
||||
// DESTINATION moves"* — i.e. it was OUT OF SCOPE for that change, which was about where restored data
|
||||
// LANDS. It was never ruled out on state-only grounds: the one comment about a `systemDataPath`
|
||||
// fallback belonged to `PlaceOffsiteRestore` and concerned merging bulk USERDATA onto the SSD, and
|
||||
// R-356 deliberately overruled even that. This function's own documented exclusion is
|
||||
// `cfg.Paths.DataDir` — the ROOTFS — which is a different filesystem entirely.
|
||||
//
|
||||
// SO §6.3's [DESIGN] RULE APPLIES, AND IT NOW HAS A FOURTH CONSUMER: "the restore destination is
|
||||
// resolved by the same rule as the capture destination — the drive if the app declares one, the system
|
||||
// data path otherwise."
|
||||
//
|
||||
// BUT THE TWO CALLERS ASK DIFFERENT QUESTIONS, and answering both with one predicate is the R-356
|
||||
// defect itself. So the fallback is scoped:
|
||||
//
|
||||
// - unitOnly=true (the R-87 proof): may fall back. `07` §7 records as [FACT] that a driveless app's
|
||||
// recovery unit ALREADY sits on `systemDataPath` indefinitely — "the SSD-only system-data
|
||||
// fallback" — and that the same-device placement is "intended, not a defect". The scratch is
|
||||
// bounded by that unit's own size and is deleted on every path.
|
||||
// - unitOnly=false (the customer's scratch): must NOT. A full restore pulls the app's bulk userdata,
|
||||
// and `07` §2.2 makes the internal SSD a STATE-ONLY tier. This is exactly the case the deleted
|
||||
// `PlaceOffsiteRestore` comment worried about, and the R-252 refusal below stays correct for it.
|
||||
//
|
||||
// The headroom gate still applies on the fallback path — it is the caller's `unitOnlyHeadroom`, which
|
||||
// refuses when the floor is not met, so a small system disk is protected by the same floor as a drive.
|
||||
func (m *Manager) offboxScratchDirIn(stack string, rootFor func(string) string, unitOnly bool) (scratch, nsRoot string, err error) {
|
||||
scratchFor := func(root string) (string, string) {
|
||||
return filepath.Join(rootFor(root), stack), m.namespaceRoot(root)
|
||||
}
|
||||
@@ -229,6 +266,16 @@ func (m *Manager) offboxScratchDirIn(stack string, rootFor func(string) string)
|
||||
}
|
||||
}
|
||||
}
|
||||
// (4) R-414: a UNIT-ONLY restore falls back to the system data path, which is where a driveless
|
||||
// app's unit already lives. Deliberately AFTER the network last-resort: a registered drive,
|
||||
// even a network one, is still a better scratch for ownership fidelity than the system disk.
|
||||
if unitOnly {
|
||||
if sysPath := strings.TrimSpace(m.cfg.Paths.SystemDataPath); sysPath != "" {
|
||||
s, nr := scratchFor(sysPath)
|
||||
m.logger.Printf("[INFO] [offbox] %s: no registered data drive — unit-only scratch falls back to the system data path %s (R-414; the unit already lives there)", stack, sysPath)
|
||||
return s, nr, nil
|
||||
}
|
||||
}
|
||||
// R-252: name the reason AND the way to act on it. This refusal is what a rebuilt box hits — the
|
||||
// drives are physically fine and still mounted, it is their REGISTRATION that the destroyed guest
|
||||
// took with it — and until v0.207.0 it said only that a drive was missing, which reads like data
|
||||
@@ -270,6 +317,27 @@ func (m *Manager) RestoreOffboxScratch(ctx context.Context, stack string, full b
|
||||
if !m.OffboxConfigured() {
|
||||
return fmt.Errorf("off-box backup not configured")
|
||||
}
|
||||
// R-411/R-408 — THE SINGLE-WRITER FLAG, and it must be taken HERE, before anything touches the
|
||||
// repository.
|
||||
//
|
||||
// WHAT IT COSTS TO OMIT IT, measured on demo-hp 2026-08-31 and not reasoned about: this function
|
||||
// runs `offboxSnapshotSize` for a full restore, which shells `restic stats` — and **`stats` TAKES
|
||||
// A REPOSITORY LOCK** (clean-room test: nothing else running, four invocations, the sampler reads
|
||||
// `locks=1`). Without this flag the integrity check is not blocked, starts, meets that lock, and
|
||||
// `resticStep` escalates to `unlock --remove-all` — the argv sampler caught `restore …` and
|
||||
// `unlock --remove-all` in the SAME sample at 20:50:51 — while logging *"a stale exclusive lock
|
||||
// left by a previous crash"*. There was no crash. `resticStep`'s own safety argument is that the
|
||||
// in-process mutex proves no sibling is live; this is the caller that made that false.
|
||||
//
|
||||
// BEFORE the snapshot lookup and the size probe, deliberately: a flag taken after the probe
|
||||
// protects nothing, because the probe is what takes the lock.
|
||||
//
|
||||
// The refusal shape matches the five siblings, so the handler's Hungarian wording is unchanged and
|
||||
// `restoreOpBlocked()` still refuses a second press exactly as it does today.
|
||||
if err := m.acquireRunning(); err != nil {
|
||||
return err
|
||||
}
|
||||
defer m.releaseRunning()
|
||||
if !isSafeStackName(stack) {
|
||||
return fmt.Errorf("invalid stack name")
|
||||
}
|
||||
@@ -425,6 +493,19 @@ func (m *Manager) OffboxRestorePrepareFull(ctx context.Context, stack string) (s
|
||||
if !m.OffboxConfigured() {
|
||||
return "", fmt.Errorf("off-box backup not configured")
|
||||
}
|
||||
// R-411 — THE SECOND ENTRY POINT, and the one the customer's UI actually reaches FIRST.
|
||||
//
|
||||
// The full restore is TWO HTTP requests: this one computes the size for the confirm screen, and a
|
||||
// later one does the restore. They are separate calls, so the flag taken in RestoreOffboxScratch
|
||||
// does not cover this, and NOTHING nests. `offboxSnapshotSize` below shells `restic stats`, which
|
||||
// takes a repository lock — so without this, the collision R-411 records is still reachable
|
||||
// through the ordinary two-step flow even after the restore itself is flagged.
|
||||
//
|
||||
// Found by re-reading the call graph while fixing the other one, not by the original report.
|
||||
if err := m.acquireRunning(); err != nil {
|
||||
return "", err
|
||||
}
|
||||
defer m.releaseRunning()
|
||||
if !isSafeStackName(stack) {
|
||||
return "", fmt.Errorf("invalid stack name")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user