From c9b4b82dc78206391615f45cfca17aa39fb86143 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 24 Oct 2026 20:29:59 +0200 Subject: [PATCH] =?UTF-8?q?R-1253=20(P2):=20an=20export=20that=20stops=20i?= =?UTF-8?q?ts=20app=20refuses=20while=20another=20operation=20(a=20restore?= =?UTF-8?q?)=20holds=20the=20app-stop=20marker=20=E2=80=94=20its=20Undo=20?= =?UTF-8?q?is=20never=20replaced.=20Commit=20only.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 16 ++++++ controller/cmd/controller/main.go | 5 +- .../controller/r1253_export_marker_test.go | 57 +++++++++++++++++++ controller/internal/appexport/export.go | 10 ++++ .../internal/appexport/r1253_busy_test.go | 48 ++++++++++++++++ controller/internal/backup/appstop_marker.go | 40 +++++++++++++ controller/internal/i18n/locales/en.json | 1 + controller/internal/i18n/locales/hu.json | 1 + controller/scripts/i18n_go_keys.json | 1 + 9 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 controller/cmd/controller/r1253_export_marker_test.go create mode 100644 controller/internal/appexport/r1253_busy_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d9c9d8f..58637d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,22 @@ brief 2026-10-24, Part 4 (commit only; the release rides the next normal train). `…OnlyWhenTheAgentWaitsAndAnAppRuns`, `…QBLimitRuleSkipsAHeldLimit`, `…TheTwoAppsReadAndWriteTheirLimit`, `TestLineTestRunOutlastsTheClientTimeout`, `TestStartUpwatchRecoversTheHoldFirst`, `TestUpwatchHoldGetsBothAgentCalls`. +### An export never replaces another operation's restore record (R-1253, P2; review round 2 HUNT-1 H3) — 2026-10-24 night, commit only + +- **An export with „stop the app first" no longer overwrites a running restore's crash record.** The app-stop marker + has one slot; the exporter's `Begin` wrote it unconditionally, so a stop-first export during another app's restore + replaced the restore's record and its Undo, and a power cut then left the restored app on mixed data with no + put-back. Now the guard knows the operation of THIS process that holds the marker (`BusyWith`), `BeginExclusive` + refuses to replace it (`ErrAppStopBusy`), the export adapter uses it, and `StartExport` refuses a stop-first export + before anything starts („err.appexport.another_operation_running", HTTP 409). A marker a crash left does not block + (nothing in this process will end it; Recover already ran). **Picked over one marker per app** because that changes + the marker's format, Recover, `HeldStacks`, `SetUndo` and every caller; this touches the exporter and one guard method. + The other direction (a restore or the night dump replacing an export's marker) needs no refusal: the replaced record + only owed the exported app a restart, which the boot sweep gives every app the household wants running. +- Tests (each seen failing with the refusal taken out): `TestR1253_AnExportNeverReplacesARestoresMarker` (main: the + review's two Begins through the real adapter; a crash-left marker does not block), + `TestR1253_AStopFirstExportWaitsForTheOtherOperation` (appexport: refused before any mark or stop). + ### A rejected backup block never deletes second-drive copies (R-1252, P2; review round 2 HUNT-2 H1, H2, H9) — 2026-10-24 night, commit only - **One wrong line in an app's backup block no longer deletes its second-drive copy.** LoadMetadata rejects the whole diff --git a/controller/cmd/controller/main.go b/controller/cmd/controller/main.go index d576be6..de8c2fd 100644 --- a/controller/cmd/controller/main.go +++ b/controller/cmd/controller/main.go @@ -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...) } diff --git a/controller/cmd/controller/r1253_export_marker_test.go b/controller/cmd/controller/r1253_export_marker_test.go new file mode 100644 index 0000000..5981f45 --- /dev/null +++ b/controller/cmd/controller/r1253_export_marker_test.go @@ -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) + } +} diff --git a/controller/internal/appexport/export.go b/controller/internal/appexport/export.go index 852c167..9373693 100644 --- a/controller/internal/appexport/export.go +++ b/controller/internal/appexport/export.go @@ -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) diff --git a/controller/internal/appexport/r1253_busy_test.go b/controller/internal/appexport/r1253_busy_test.go new file mode 100644 index 0000000..ea3f3b9 --- /dev/null +++ b/controller/internal/appexport/r1253_busy_test.go @@ -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) + } +} diff --git a/controller/internal/backup/appstop_marker.go b/controller/internal/backup/appstop_marker.go index 003b108..833ec31 100644 --- a/controller/internal/backup/appstop_marker.go +++ b/controller/internal/backup/appstop_marker.go @@ -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 diff --git a/controller/internal/i18n/locales/en.json b/controller/internal/i18n/locales/en.json index da81c89..721c3a2 100644 --- a/controller/internal/i18n/locales/en.json +++ b/controller/internal/i18n/locales/en.json @@ -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", diff --git a/controller/internal/i18n/locales/hu.json b/controller/internal/i18n/locales/hu.json index e7282a7..7d9a76e 100644 --- a/controller/internal/i18n/locales/hu.json +++ b/controller/internal/i18n/locales/hu.json @@ -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", diff --git a/controller/scripts/i18n_go_keys.json b/controller/scripts/i18n_go_keys.json index b31fdb7..490255e 100644 --- a/controller/scripts/i18n_go_keys.json +++ b/controller/scripts/i18n_go_keys.json @@ -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.",