R-1253 (P2): an export that stops its app refuses while another operation (a restore) holds the app-stop marker — its Undo is never replaced. Commit only.

2026-10-24 20:29:59 +02:00
parent 518a13592d
commit c9b4b82dc7
9 changed files with 178 additions and 1 deletions
@@ -3601,9 +3601,12 @@ func (a *exportAdapter) GetStacksBaseDir() string {
 type exportStopGuard struct{ g *backup.AppStopGuard }

 func (a exportStopGuard) Begin(opID string, stacks []string) error {
-	return a.g.Begin(opID, backup.ReasonAppExport, stacks)
+	// R-1253: never replace the marker another operation (a restore) holds — its Undo is the only crash record.
+	return a.g.BeginExclusive(opID, backup.ReasonAppExport, stacks)
 }

+func (a exportStopGuard) BusyWith() string { return a.g.BusyWith() }
+
 func (a exportStopGuard) End() { a.g.End() }

 func (a exportStopGuard) ReleaseFailed(stacks ...string) { a.g.ReleaseFailed(stacks...) }
@@ -0,0 +1,57 @@
+package main
+
+import (
+	"errors"
+	"io"
+	"log"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"gitea.dooplex.hu/admin/felhom-controller/internal/backup"
+)
+
+// R-1253 (review round 2, HUNT-1 H3): an export with „stop the app first" during another app's restore replaced the
+// ONE app-stop marker — the restore's crash record and its Undo were lost, and a power cut then left the restored app
+// on mixed data with no put-back. The test is the review's mechanism with two Begins on one guard, through the
+// exporter's real adapter. RED-PROOF (2026-10-24): with exportStopGuard.Begin calling Begin instead of BeginExclusive,
+// this fails on „the restore's Undo is gone from the marker".
+func TestR1253_AnExportNeverReplacesARestoresMarker(t *testing.T) {
+	dir := t.TempDir()
+	marker := filepath.Join(dir, "appstop-state.json")
+	g := backup.NewAppStopGuard(marker, log.New(io.Discard, "", 0))
+	if err := g.Begin("unit-restore:bookstack", backup.ReasonUnitRestore, []string{"bookstack"}); err != nil {
+		t.Fatal(err)
+	}
+	undo := filepath.Join(dir, "backups", "restore-undo", "bookstack", "20261024T200000Z")
+	if err := g.SetUndo("bookstack", undo); err != nil {
+		t.Fatal(err)
+	}
+	ex := exportStopGuard{g: g}
+	if busy := ex.BusyWith(); busy != "unit-restore:bookstack" {
+		t.Fatalf("the export must see the restore holding the marker, got %q", busy)
+	}
+	err := ex.Begin("app-export:immich", []string{"immich"})
+	if !errors.Is(err, backup.ErrAppStopBusy) {
+		t.Fatalf("the export's Begin must be refused while the restore holds the marker, got %v", err)
+	}
+	b, _ := os.ReadFile(marker)
+	if !strings.Contains(string(b), undo) || strings.Contains(string(b), "immich") {
+		t.Fatalf("the restore's Undo is gone from the marker (or the export replaced it): %s", b)
+	}
+	// the restore ends → the export may run
+	g.End()
+	if err := ex.Begin("app-export:immich", []string{"immich"}); err != nil {
+		t.Fatalf("after the restore ended the export must start: %v", err)
+	}
+	ex.End()
+	// a marker a CRASH left (a new process over the same file) does not block: nothing in this process will End it
+	if err := g.Begin("volume-dump:paperless", backup.ReasonVolumeDump, []string{"paperless"}); err != nil {
+		t.Fatal(err)
+	}
+	fresh := exportStopGuard{g: backup.NewAppStopGuard(marker, log.New(io.Discard, "", 0))}
+	if busy := fresh.BusyWith(); busy != "" {
+		t.Fatalf("a crash-left marker is not a live operation, got %q", busy)
+	}
+}
@@ -120,6 +120,9 @@ type appStopGuard interface {
 	// same reason Begin is: the guard owns the "we are holding this app" fact, and a second owner
 	// would drift from it.
 	ReleaseFailed(stacks ...string)
+	// BusyWith (R-1253) names another operation that holds the marker now ("" = none): an export that stops its app
+	// refuses then, because the marker has one slot and a restore's crash record (its Undo) would be replaced.
+	BusyWith() string
 }

 // NewExporter creates a new export/import engine.
@@ -183,6 +186,13 @@ func (e *Exporter) StartExport(req ExportRequest) error {
 		e.debugf("StartExport rejected: stack %q is not deployed", req.StackName)
 		return fmt.Errorf("stack %q is not deployed", req.StackName)
 	}
+	if req.StopApp && e.stopGuard != nil {
+		if busy := e.stopGuard.BusyWith(); busy != "" {
+			e.mu.Unlock()
+			e.logger.Printf("[WARN] Export of %s refused: %s holds the app-stop marker — an export that stops the app would replace its crash record (R-1253)", req.StackName, busy)
+			return util.MsgError("err.appexport.another_operation_running")
+		}
+	}

 	e.debugf("StartExport: stack=%q dest=%q password=%v stopApp=%v",
 		req.StackName, req.DestDrive, req.Password != "", req.StopApp)
@@ -0,0 +1,48 @@
+package appexport
+
+import (
+	"io"
+	"log"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// busyGuard is the app-stop seam with another operation holding the marker.
+type busyGuard struct{ begins []string }
+
+func (g *busyGuard) Begin(opID string, _ []string) error {
+	g.begins = append(g.begins, opID)
+	return nil
+}
+func (g *busyGuard) End()                    {}
+func (g *busyGuard) ReleaseFailed(...string) {}
+func (g *busyGuard) BusyWith() string        { return "unit-restore:bookstack" }
+
+// R-1253: an export that stops its app is refused BEFORE anything starts while another operation holds the marker;
+// an export that does not stop its app writes no marker and still runs. RED-PROOF (2026-10-24): with the BusyWith check
+// removed from StartExport, the stop-first export starts (StartExport returns nil) and this fails.
+func TestR1253_AStopFirstExportWaitsForTheOtherOperation(t *testing.T) {
+	stackDir := filepath.Join(t.TempDir(), "immich")
+	if err := os.MkdirAll(stackDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	prov := &rtProvider{stackDir: stackDir, stacksDir: filepath.Dir(stackDir), deployed: true, running: true}
+	e := NewExporter(prov, log.New(io.Discard, "", 0), "test")
+	g := &busyGuard{}
+	e.SetStopGuard(g)
+	err := e.StartExport(ExportRequest{StackName: "immich", DestDrive: t.TempDir(), StopApp: true})
+	if err == nil {
+		t.Fatal("a stop-first export must be refused while a restore holds the app-stop marker")
+	}
+	if k, ok := err.(interface{ Key() string }); !ok || k.Key() != "err.appexport.another_operation_running" {
+		t.Fatalf("the refusal must carry its household sentence key, got %v", err)
+	}
+	if !strings.Contains(err.Error(), "visszaállítás") {
+		t.Fatalf("the Hungarian sentence must name the cause: %q", err.Error())
+	}
+	if len(g.begins) != 0 {
+		t.Fatalf("nothing may be marked or stopped, begins=%v", g.begins)
+	}
+}
@@ -138,6 +138,11 @@ type AppStopGuard struct {
 	holds         AppStopHolds
 	undoReachable func(stack, dir string) bool

+	// liveOp (R-1253) is the operation of THIS process that holds the marker now (set by Begin, cleared by End). The
+	// marker has one slot; BeginExclusive refuses to replace a live one. A marker left by a crash has no live op.
+	liveMu sync.Mutex
+	liveOp string
+
 	// suppressed (R-330) is the in-memory "the alarm must not fire for these, we are holding them"
 	// set. It is deliberately NOT persisted: after a restart the guard no longer holds anything —
 	// Recover() either brings the apps back or leaves them genuinely down, and a down app must alarm.
@@ -315,6 +320,9 @@ func (g *AppStopGuard) Begin(opID string, reason AppStopReason, stackNames []str
 	}); err != nil {
 		return err
 	}
+	g.liveMu.Lock()
+	g.liveOp = opID
+	g.liveMu.Unlock()
 	// R-330: the marker is on disk, so the caller is now cleared to stop these apps — which is
 	// exactly the moment the app-down alarm must stop counting them. Marked AFTER the write, so a
 	// Begin that failed (and therefore stopped nothing) suppresses nothing either.
@@ -335,6 +343,9 @@ func (g *AppStopGuard) End() {
 	// still owes its suppressed stacks a release — otherwise an unwired-path guard would hold them
 	// until appStopMaxHold.
 	g.releaseStarted()
+	g.liveMu.Lock()
+	g.liveOp = ""
+	g.liveMu.Unlock()
 	if g.path == "" {
 		return
 	}
@@ -343,6 +354,35 @@ func (g *AppStopGuard) End() {
 	}
 }

+// ErrAppStopBusy (R-1253): another operation of this process holds the app-stop marker; replacing it would lose that
+// operation's crash record (a restore's Undo among it).
+var ErrAppStopBusy = errors.New("another operation holds the app-stop marker")
+
+// BusyWith names the operation of THIS process that holds the marker now ("" = none). A marker a crash left is not
+// busy: nothing in this process will End it, and the next operation may replace it (Recover already ran).
+func (g *AppStopGuard) BusyWith() string {
+	if g == nil {
+		return ""
+	}
+	g.liveMu.Lock()
+	defer g.liveMu.Unlock()
+	return g.liveOp
+}
+
+// BeginExclusive is Begin that refuses while another operation of this process holds the marker (R-1253: an export
+// with „stop the app first" during another app's restore replaced the restore's record and its Undo). Used by the
+// exporter; the backup manager's own operations are already serialised by its running flag.
+func (g *AppStopGuard) BeginExclusive(opID string, reason AppStopReason, stackNames []string) error {
+	if g == nil {
+		return nil
+	}
+	if busy := g.BusyWith(); busy != "" && busy != opID {
+		g.logger.Printf("[WARN] [appstop] %s refused: %s holds the app-stop marker (R-1253)", opID, busy)
+		return fmt.Errorf("%w: %s", ErrAppStopBusy, busy)
+	}
+	return g.Begin(opID, reason, stackNames)
+}
+
 // Recover restarts any apps left stopped by an operation that died before restarting them, then
 // clears the marker. Call ONCE at startup, and — critically — call it to COMPLETION before the boot
 // reconciler is launched, so an app this marker explains is not also reported as an unexplained boot
@@ -1250,6 +1250,7 @@
  "err.after_start.failed": "Saving did not work. Try again.",
  "err.backup.restore_drive_gone": "The drive “%s” cannot be reached, so the restore could not start. Connect the drive again, then start the restore again.",
  "err.stacks.remove_keep_data_while_support": "While support is working on this app, its data cannot be deleted — you can remove the app and keep its data.",
+  "err.appexport.another_operation_running": "Another operation (for example a restore) is holding an app stopped now, so an export that stops the app cannot start. Try again when that operation has finished.",
  "err.appexport.a_mentes_nem_tartalmaz_alkalmazasadatot_0": "the backup holds no app data (0 data folders, 0 volumes for an app that stores data)",
  "err.appexport.a_z_adatkonyvtar_tartalma_hianyzik_a": "the contents of data folder %q are missing from the package",
  "err.appexport.a_z_kotet_adata_hianyzik_a": "the data of volume %q is missing from the package",
@@ -1245,6 +1245,7 @@
  "err.after_start.failed": "A mentés nem sikerült. Próbáld meg újra.",
  "err.backup.restore_drive_gone": "A(z) „%s” meghajtó nem érhető el, ezért a visszaállítás nem tudott elindulni. Csatlakoztasd újra a meghajtót, majd indítsd el újra a visszaállítást.",
  "err.stacks.remove_keep_data_while_support": "Amíg az ügyfélszolgálat foglalkozik az alkalmazással, az adatai nem törölhetők — az alkalmazást az adatai megtartásával eltávolíthatod.",
+  "err.appexport.another_operation_running": "Egy másik művelet (például egy visszaállítás) éppen leállítva tart egy alkalmazást, ezért most nem indítható olyan exportálás, amely leállítja az alkalmazást. Próbáld újra, amikor az a művelet befejeződött.",
  "err.appexport.a_mentes_nem_tartalmaz_alkalmazasadatot_0": "a mentés nem tartalmaz alkalmazásadatot (0 adatkönyvtár, 0 kötet egy adattárolós alkalmazásnál)",
  "err.appexport.a_z_adatkonyvtar_tartalma_hianyzik_a": "a(z) %q adatkönyvtár tartalma hiányzik a csomagból",
  "err.appexport.a_z_kotet_adata_hianyzik_a": "a(z) %q kötet adata hiányzik a csomagból",
@@ -12,6 +12,7 @@
    "err.backup.restore_failed_held_mixed": "BORN AS A KEY (R-893, 2026-10-08, security review) -- a restore that stopped half-way (placement, volume replay or DB start failed) after files/volumes/version had moved; the app is HELD. Pinned by internal/backup/r893_mixed_restore_test.go.",
    "note.restore.interrupted_put_back_waiting": "BORN AS A KEY (R-1251, 2026-10-24) -- the restore record sentence for an interrupted restore whose put-back waits for the app's drive. Never a Go literal; pinned by TestR1251_TheRecordSaysThePutBackWaits.",
    "note.tier2.settings_unreadable_kept": "BORN AS A KEY (R-1252, 2026-10-24) -- the second-drive status note when a rejected backup block made the night copy keep every copy. Never a Go literal; pinned by r1252_rejected_block_keeps_copies_test.go.",
+    "err.appexport.another_operation_running": "BORN AS A KEY (R-1253, 2026-10-24) -- an export that stops its app is refused while another operation holds the app-stop marker. Never a Go literal; pinned by TestR1253_AnExportNeverReplacesARestoresMarker.",
    "hold.putback_pending": "BORN AS A KEY (R-1251, 2026-10-24) -- the hold sentence for an app whose cut restore waits for its drive before its pre-restore data is put back (HoldReasonPutBackPending). Never a Go literal; pinned by internal/backup/r1251_putback_waits_test.go.",
    "note.reconstitute.held_mixed": "BORN AS A KEY (R-893, 2026-10-08) -- the hold sentence for that held app (HoldReasonRestoreMixed). Never a Go literal; pinned by internal/backup/r893_mixed_restore_test.go.",
    "recovery.older_unchecked": "BORN AS A KEY (R-304, 2026-10-08) -- a NEW sentence of the recovery screen for the agent's 424 (earlier sealed packages not all checked), never a Go literal; pinned by TestR304_OlderUnchecked_IsNotAWrongCode.",