diff --git a/CHANGELOG.md b/CHANGELOG.md index db2b5a7..e748103 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,13 @@ background retry for strays and a failed start-up check; tests switch it off (`tier2RetryOff` — a timer outliving a test would reach the host's real Docker). N9: the R-1187 (c) refusal no longer advises removing an unhealthy app (its data may be newer than the bundle) — „kérd az üzemeltető segítségét". Tests `TestStrayCopy_AStrayClears…`, `…KeepsThis - ProcessesOwnCopy`, `…SameDestinationWaits`, `TestR1180_ADirectRunTier2…`, each red-proved; `-race` clean. Tests `TestStrayCopy_*` (backup, web, + ProcessesOwnCopy`, `…SameDestinationWaits`, `TestR1180_ADirectRunTier2…`, each red-proved; `-race` clean. +- **Two races closed (the commit's own security review).** The start-up check lists the labelled containers, then removes + them — a copy of the same destination could start between: each removal now holds that destination's lock (TryLock; + held → this process copies there → skipped). `RunTier2` checked for a restore, then recorded the copy later — a restore + beginning between saw nothing: the check and the record are now one step under the restore record's lock + (`beginTier2Run`), and every leg asks again before it copies. Tests `TestStrayCopy_TheStartUpCheckSkipsADestination + BeingCopied`, `TestR1180_ARestoreBeginningMidRunStopsTheNextLeg`, `TestR1180_ARunInFlightBlocks…`, each red-proved. Tests `TestStrayCopy_*` (backup, web, wiring; Docker-free seams), each red-proved; 9202 proof in felhom.eu `audits/night-2026-10-23/P1-straycopy/`. ## Unreleased — the second-drive copy no longer squeezes the apps' memory (R-1214), 2026-10-22 night diff --git a/controller/internal/backup/r1180_mirror_during_restore_test.go b/controller/internal/backup/r1180_mirror_during_restore_test.go index c5d41eb..073f6da 100644 --- a/controller/internal/backup/r1180_mirror_during_restore_test.go +++ b/controller/internal/backup/r1180_mirror_during_restore_test.go @@ -80,3 +80,46 @@ func TestR1180_ADirectRunTier2NeverRunsDuringARestore(t *testing.T) { } _ = target } + +// Commit security review 2026-10-23 night: a restore may begin while a run is already past its first check. The run +// records itself in one step under the restore record's lock, and every leg asks again. RED-PROOF: with the per-leg +// check removed, ARestoreBeginningMidRun fails (the photo leg is mirrored over the restore) — seen failing 2026-10-23. +func TestR1180_ARestoreBeginningMidRunStopsTheNextLeg(t *testing.T) { + m, src, _, prov := newTier2V2(t, "app") + prov.has["app"] = true + prov.binds["app"] = []ClassifiedBind{mHDD("appdata/app/photos")} + m.stackProvider = r1180Provider{prov} + var dsts []string + m.tier2Mirror = func(s, d string) error { + dsts = append(dsts, d) + if len(dsts) == 1 { + m.BeginRestoreOp("tier2-whole-restore", "app") // the household starts a restore during the unit leg + } + return rsyncMirror(s, d) + } + mustWrite(t, filepath.Join(src, "appdata", "app", "photos", "a.jpg"), "a") + if err := m.RunTier2("app"); err != nil { + t.Fatal(err) + } + if len(dsts) != 1 { + t.Fatalf("after a restore began, no further leg may be mirrored: %v", dsts) + } + if _, blocked := m.Tier2CopyBlocksRestore(); blocked { + t.Fatal("the run must have ended its record") + } +} + +func TestR1180_ARunInFlightBlocksARestoreAndARestoreBlocksARun(t *testing.T) { + m := &Manager{} + if !m.beginTier2Run("app") { + t.Fatal("no restore: the run must begin") + } + if _, blocked := m.Tier2CopyBlocksRestore(); !blocked { + t.Fatal("a run in flight must refuse a restore") + } + tier2CopyEnd("app") + m.opRunning, m.opStack = true, "app" + if m.beginTier2Run("app") { + t.Fatal("a restore of the app must refuse the run") + } +} diff --git a/controller/internal/backup/tier2.go b/controller/internal/backup/tier2.go index ddf6be1..4796e02 100644 --- a/controller/internal/backup/tier2.go +++ b/controller/internal/backup/tier2.go @@ -342,10 +342,13 @@ func (m *Manager) tier2ReconcileRoots(destBase string, roots, legRels []string) func (m *Manager) RunTier2(stackName string) error { // R-1180 for EVERY caller (review 2026-10-23 night, N5): the household's „Tier 2 on" save and the debug run call // this directly, outside RunAllTier2's skip — a mirror (rsync --delete) must never start during a restore of the app. - if m.restoreRunningFor(stackName) { + // The check and the copy's record are one step under m.mu (commit security review 2026-10-23 night): a restore that + // begins after this point sees the copy (Tier2CopyBlocksRestore), and every leg below asks again before it copies. + if !m.beginTier2Run(stackName) { m.logger.Printf("[WARN] [backup] Tier 2 for %s not started — a restore of it is running; it is mirrored at the next run (R-1180)", stackName) return nil } + defer tier2CopyEnd(stackName) // Customer turned Tier 2 off for this app (config panel) — skip without touching status. if m.settings != nil { if cd := m.settings.GetCrossDriveConfig(stackName); cd != nil && cd.UserDisabled { @@ -443,6 +446,9 @@ func (m *Manager) RunTier2(stackName string) error { m.logger.Printf("[WARN] [backup] Tier 2 %s: unit leg SKIPPED — the recovery unit on the source drive lists no database dumps and no volume tars, while the existing copy at %s does. The copy was PRESERVED rather than replaced with an empty one (R-403). The other legs continue.", stackName, destUnit) warns = append(warns, m.note(tier2UnitPreservedKey)) + } else if m.restoreRunningFor(stackName) { + m.logger.Printf("[WARN] [backup] Tier 2 %s stopped before the unit leg — a restore of it began (R-1180)", stackName) + return nil } else if err := mirror(unitDir, destUnit); err != nil { m.recordTier2Failure(stackName, target, err) if m.tier2Notify != nil { @@ -460,6 +466,10 @@ func (m *Manager) RunTier2(stackName string) error { warns = append(warns, m.note("note.tier2.emptied_kept", lg.DestRel)) continue } + if m.restoreRunningFor(stackName) { + m.logger.Printf("[WARN] [backup] Tier 2 %s stopped before leg %s — a restore of it began (R-1180)", stackName, lg.DestRel) + return nil + } if err := mirror(lg.Src, dst); err != nil { m.recordTier2Failure(stackName, target, err) if m.tier2Notify != nil { @@ -930,11 +940,27 @@ func dirSizeBytes(dir string) int64 { return size } +// beginTier2Run records a second-drive run of stack as in flight — unless a restore of it runs — in ONE step under m.mu, +// the lock the restore record lives under. The caller ends it with tier2CopyEnd(stack). +func (m *Manager) beginTier2Run(stack string) bool { + m.mu.Lock() + defer m.mu.Unlock() + if m.restoreRunningForLocked(stack) { + return false + } + tier2CopyBegin(stack) + return true +} + // restoreRunningFor reports whether a restore of stack is in flight (the restore record the pages follow, R-550). // The shares restores are recorded under their display name. func (m *Manager) restoreRunningFor(stack string) bool { m.mu.Lock() defer m.mu.Unlock() + return m.restoreRunningForLocked(stack) +} + +func (m *Manager) restoreRunningForLocked(stack string) bool { if !m.opRunning { return false } diff --git a/controller/internal/backup/tier2_copy_lifecycle.go b/controller/internal/backup/tier2_copy_lifecycle.go index 96f7dbd..16d7089 100644 --- a/controller/internal/backup/tier2_copy_lifecycle.go +++ b/controller/internal/backup/tier2_copy_lifecycle.go @@ -232,6 +232,11 @@ func tier2RetryOnce(ctx context.Context) bool { // tier2RemoveCopyQuiet is the retry's removal: a failure keeps the stray without starting another retry. func tier2RemoveCopyQuiet(ctx context.Context, name string) { + lk := tier2CopyLock(name) + if !lk.TryLock() { + return // a copy of that destination runs now: it removes the stray itself before it starts + } + defer lk.Unlock() rctx, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() out, err := tier2Docker(rctx, "rm", "-f", name) @@ -278,15 +283,23 @@ func clearLeftoverTier2CopiesOnce(ctx context.Context) error { } } tier2Copies.Unlock() + // The check above and the removal below are not atomic: a copy of the same destination may start in between. Each + // removal holds that destination's lock (TryLock: a copy of it runs in this process → skipped), so it can never hit + // a copy this process starts (commit security review 2026-10-23 night). if len(names) == 0 { tier2Logf("[INFO] [tier2] start-up: no leftover second-drive copy container") return nil } failed := 0 for _, n := range names { + lk := tier2CopyLock(n) + if !lk.TryLock() { + continue // this process copies into that destination right now — its own container, not a leftover + } rctx, rcancel := context.WithTimeout(ctx, 60*time.Second) rout, rerr := tier2Docker(rctx, "rm", "-f", n) rcancel() + lk.Unlock() if rerr != nil { failed++ tier2Logf("[ERROR] [tier2] start-up: could not remove the leftover copy container %s: %v %s", n, rerr, strings.TrimSpace(string(rout))) diff --git a/controller/internal/backup/tier2_copy_lifecycle_test.go b/controller/internal/backup/tier2_copy_lifecycle_test.go index ae8e70f..d6dea03 100644 --- a/controller/internal/backup/tier2_copy_lifecycle_test.go +++ b/controller/internal/backup/tier2_copy_lifecycle_test.go @@ -305,3 +305,24 @@ func TestStrayCopy_ASecondCopyOfTheSameDestinationWaits(t *testing.T) { t.Fatal(err) } } + +// Commit security review 2026-10-23 night: the start-up check's list and its removals are not atomic — a copy of the +// same destination may start between them. A destination this process copies into (its lock held) is skipped. +// RED-PROOF: with the TryLock skip removed, this fails — seen failing 2026-10-23 night. +func TestStrayCopy_TheStartUpCheckSkipsADestinationBeingCopied(t *testing.T) { + _, life := strayCopySeams(t, t.TempDir()+"/", nil, func(args []string) ([]byte, error) { + if args[0] == "ps" { + return []byte("felhom-tier2-copy-busy-00000003\n"), nil + } + return nil, nil + }) + lk := tier2CopyLock("felhom-tier2-copy-busy-00000003") + lk.Lock() // a copy of that destination is starting in this process (not yet marked running) + defer lk.Unlock() + if err := clearLeftoverTier2CopiesOnce(context.Background()); err != nil { + t.Fatal(err) + } + if strings.Contains(life.all(), "rm -f felhom-tier2-copy-busy-00000003") { + t.Fatalf("the check removed a container of a destination this process is copying into:\n%s", life.all()) + } +}