From b2c0b973fb00bb6f0efb7a98331bbdcb51e79534 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:06:18 +0200 Subject: [PATCH 01/15] test: pin the PVE-only host that a leftover PBS directory turns dual Removing proxmox-backup-server takes away everything the package ships: its binary, its share directory, its dpkg stanza. It does not take away /etc/proxmox-backup or /var/lib/proxmox-backup. Those are created at runtime, belong to no package (dpkg -S finds nothing for either, measured on PBS 3.4.9 and 4.2.0) and the server postrm leaves them alone even on purge. The detection ladder ends on a directory rung, so on a host in that state a directory that outlived its product is the only evidence of PBS, and the verdict is dual. The run then aborts on proxmox-backup-manager, a command the host does not have, and the abort discards the PVE payload that was already collected (issue #315). These tests state the verdict the host deserves and fail today. They cover the live shape, the same host under SYSTEM_ROOT_PREFIX, the second candidate directory alone (deleting /etc/proxmox-backup changed nothing because the ladder walks the whole list), and an empty version file. Two of them are non-regressions rather than new claims: a coinstalled host must stay dual (issue #197, the request that produced dual support) and an installed PBS under a prefix must still be found without running a command (issue #255). DetectionStep.Residual and EnvironmentInfo.PVEResidual/PBSResidual land here so the red is behavioural rather than a build failure. Nothing populates them yet. No release note: this commit changes nothing an operator can observe. --- internal/environment/detect.go | 32 ++- internal/environment/detect_residue_test.go | 243 ++++++++++++++++++++ 2 files changed, 264 insertions(+), 11 deletions(-) create mode 100644 internal/environment/detect_residue_test.go diff --git a/internal/environment/detect.go b/internal/environment/detect.go index 7a81df75..432b9a58 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -173,13 +173,20 @@ type EnvironmentInfo struct { PBSVersion string // PVESource and PBSSource name the marker that ended each product ladder, and - // Steps is the whole ladder that led there. They are provenance only: nothing - // branches on them. Detection stops at its first hit, so a dual verdict can rest - // on a single leftover directory on a PVE-only host, and no log used to say which - // marker decided it (issue #315). + // Steps is the whole ladder that led there. They are provenance: the verdict is + // decided by the ladder, not by reading these back. PVESource string PBSSource string Steps []DetectionStep + + // PVEResidual and PBSResidual name a marker that was found but does NOT prove the + // product is installed: a directory the package never owned, or an empty version + // file. They are empty when the product is installed, and empty when nothing at all + // was found. A non-empty residual with an absent product is the whole explanation + // for a verdict an operator did not expect (issue #315), so it is reported rather + // than discarded. + PVEResidual string + PBSResidual string } // Product labels used by the detection trace. @@ -193,13 +200,14 @@ const ( // answer it gave. A recorded run reads as "these markers missed, this one decided the // type". type DetectionStep struct { - Product string // productPVE or productPBS - Marker string // command, version-file, dpkg, cluster-db, binary, share-dir, apt-source, directory - Target string // path(s) or command consulted - Hit bool - Skipped bool // probe not run at all (command probes under a host prefix) - Version string // version the marker yielded, when it carries one - Note string // why a probe was skipped + Product string // productPVE or productPBS + Marker string // command, version-file, dpkg, cluster-db, binary, share-dir, directory + Target string // path(s) or command consulted + Hit bool + Skipped bool // probe not run at all (command probes under a host prefix) + Residual bool // marker found, but it does not prove the product is installed + Version string + Note string // why a probe was skipped, or what the residue means } // String renders the step as the single line a debug log carries. @@ -212,6 +220,8 @@ func (s DetectionStep) String() string { outcome = "HIT, version " + s.Version case s.Hit: outcome = "HIT, no version" + case s.Residual: + outcome = "residue, does not prove an install" } line := fmt.Sprintf("%s %s (%s): %s", s.Product, s.Marker, s.Target, outcome) if s.Note != "" { diff --git a/internal/environment/detect_residue_test.go b/internal/environment/detect_residue_test.go new file mode 100644 index 00000000..1240f2b0 --- /dev/null +++ b/internal/environment/detect_residue_test.go @@ -0,0 +1,243 @@ +package environment + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tis24dev/proxsave/internal/types" +) + +// pveOnlyCommandSeams makes pveversion the only command that answers, which is the +// live shape of the issue #315 host: PVE is installed and PBS is not. +func pveOnlyCommandSeams(t *testing.T) { + t.Helper() + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + if cmd == "pveversion" { + return "/usr/bin/pveversion", nil + } + return "", errors.New("executable file not found in $PATH") + }) + setValue(t, &runCommandFunc, func(string, ...string) (string, error) { + return "pve-manager/9.2.18/abcdef0123456789 (running kernel: 6.17.4-2-pve)", nil + }) +} + +// TestLeftoverPBSDirectoryDoesNotMakeAPVEHostDual is issue #315. Removing +// proxmox-backup-server takes away everything the package ships (its binary, its +// share directory, its dpkg stanza) but leaves /etc/proxmox-backup and +// /var/lib/proxmox-backup behind: they are created at runtime, belong to no package, +// and the server postrm does not remove them even on purge (measured on PBS 3.4.9 +// and 4.2.0). A directory that outlives the product it belonged to cannot be the +// evidence that the product is installed. +func TestLeftoverPBSDirectoryDoesNotMakeAPVEHostDual(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + pveOnlyCommandSeams(t) + + leftovers := []string{ + filepath.Join(tmp, "etc/proxmox-backup"), + filepath.Join(tmp, "var/lib/proxmox-backup"), + } + for _, dir := range leftovers { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + setValue(t, &pbsDirCandidates, leftovers) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE: no PBS package is installed on this host", info.Type) + } + if info.PBSVersion != "" { + t.Fatalf("PBSVersion = %q, want empty", info.PBSVersion) + } + if info.PBSSource != "" { + t.Fatalf("PBSSource = %q, want no deciding marker", info.PBSSource) + } +} + +// TestSecondLeftoverDirectoryDoesNotDecideEither guards the ladder walking its whole +// candidate list: deleting /etc/proxmox-backup while /var/lib/proxmox-backup stays +// used to leave the verdict unchanged, so an operator who followed the obvious +// remedy saw no difference. +func TestSecondLeftoverDirectoryDoesNotDecideEither(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + pveOnlyCommandSeams(t) + + varLib := filepath.Join(tmp, "var/lib/proxmox-backup") + if err := os.MkdirAll(varLib, 0o755); err != nil { + t.Fatal(err) + } + setValue(t, &pbsDirCandidates, []string{filepath.Join(tmp, "etc/proxmox-backup"), varLib}) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) + } +} + +// TestResidueIsReportedEvenThoughItDoesNotDecide: the leftovers are the whole reason +// the operator expected a different verdict, so detection must still name them. +// Dropping them silently would answer the question with nothing to check. +func TestResidueIsReportedEvenThoughItDoesNotDecide(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + pveOnlyCommandSeams(t) + + leftover := filepath.Join(tmp, "etc/proxmox-backup") + if err := os.MkdirAll(leftover, 0o755); err != nil { + t.Fatal(err) + } + setValue(t, &pbsDirCandidates, []string{leftover}) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if !strings.Contains(info.PBSResidual, leftover) { + t.Fatalf("PBSResidual = %q, want it to name %s", info.PBSResidual, leftover) + } + + step, ok := stepFor(info.Steps, productPBS, "directory") + if !ok { + t.Fatal("no PBS directory step recorded: the rung must still be walked") + } + if step.Hit { + t.Fatalf("PBS directory step = %v, want it recorded as residue, not a hit", step) + } + if !step.Residual { + t.Fatalf("PBS directory step = %v, want Residual set", step) + } + if !strings.Contains(step.String(), "residue") { + t.Fatalf("step line = %q, want it to read as residue", step.String()) + } +} + +// TestLeftoverPBSDirectoryUnderPrefixDoesNotMakeAHostDual is the same host reached +// through SYSTEM_ROOT_PREFIX (issue #255), where command probes are skipped and only +// the offline markers answer. PVE is proved by its dpkg stanza. +func TestLeftoverPBSDirectoryUnderPrefixDoesNotMakeAHostDual(t *testing.T) { + root := t.TempDir() + writeFile(t, filepath.Join(root, "var/lib/dpkg/status"), dpkgStanza("pve-manager", "9.2.18")) + for _, dir := range []string{"etc/proxmox-backup", "var/lib/proxmox-backup"} { + if err := os.MkdirAll(filepath.Join(root, dir), 0o755); err != nil { + t.Fatal(err) + } + } + + info, err := DetectWith(DetectOptions{RootPrefix: root}) + if err != nil { + t.Fatalf("DetectWith: %v", err) + } + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) + } + if info.PVEVersion != "9.2.18" { + t.Fatalf("PVEVersion = %q, want 9.2.18", info.PVEVersion) + } +} + +// TestInstalledPBSUnderPrefixIsStillDetected is the non-regression for issue #255: +// the markers a real PBS carries must still classify it without running a command. +// Measured on PBS 3.4.9 and 4.2.0: the manager binary is in /usr/sbin, the share +// directory belongs to proxmox-backup-server, and /etc/proxmox-backup/version does +// not exist on either release. +func TestInstalledPBSUnderPrefixIsStillDetected(t *testing.T) { + root := t.TempDir() + writeFile(t, filepath.Join(root, "var/lib/dpkg/status"), dpkgStanza("proxmox-backup-server", "4.2.0-1")) + writeFile(t, filepath.Join(root, "usr/sbin/proxmox-backup-manager"), "#!/bin/sh\n") + if err := os.MkdirAll(filepath.Join(root, "usr/share/proxmox-backup"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(root, "etc/proxmox-backup"), 0o700); err != nil { + t.Fatal(err) + } + + info, err := DetectWith(DetectOptions{RootPrefix: root}) + if err != nil { + t.Fatalf("DetectWith: %v", err) + } + if info.Type != types.ProxmoxBS { + t.Fatalf("Type = %v, want ProxmoxBS", info.Type) + } + if info.PBSVersion != "4.2.0-1" { + t.Fatalf("PBSVersion = %q, want 4.2.0-1", info.PBSVersion) + } +} + +// TestCoinstalledHostIsStillDual is the non-regression for issue #197, the request +// that produced dual support: a host where both products answer must keep reporting +// both. pve-test is exactly this shape, PVE 9.1.9 and PBS 4.2.0 on one node. +func TestCoinstalledHostIsStillDual(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + switch cmd { + case "pveversion": + return "/usr/bin/pveversion", nil + case "proxmox-backup-manager": + return "/usr/sbin/proxmox-backup-manager", nil + } + return "", errors.New("not found") + }) + setValue(t, &runCommandFunc, func(cmd string, _ ...string) (string, error) { + if strings.HasSuffix(cmd, "pveversion") { + return "pve-manager/9.1.9/ee7bad0a3d1546c9 (running kernel: 7.0.2-2-pve)", nil + } + return "proxmox-backup-server 4.2.5-1 running version: 4.2.0", nil + }) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxDual { + t.Fatalf("Type = %v, want ProxmoxDual", info.Type) + } + if info.Version != "pve=9.1.9,pbs=4.2.0" { + t.Fatalf("Version = %q, want the two versions combined", info.Version) + } +} + +// TestEmptyPBSVersionFileIsResidue: an existing but empty version file used to count +// as a PBS hit carrying the version "unknown". A file with nothing in it proves +// nothing about what is installed. +func TestEmptyPBSVersionFileIsResidue(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + pveOnlyCommandSeams(t) + + versionFile := filepath.Join(tmp, "etc/proxmox-backup/version") + writeFile(t, versionFile, "") + setValue(t, &pbsVersionFile, versionFile) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) + } +} + +// dpkgStanza renders the two fields dpkgPackageInstalled reads, in the layout the +// real /var/lib/dpkg/status uses (blank-line separated stanzas). +func dpkgStanza(pkg, version string) string { + return "Package: " + pkg + "\nStatus: install ok installed\nVersion: " + version + "\n\n" +} From df46e51e6c294ade457a10e651244e16398717ce Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:24:48 +0200 Subject: [PATCH 02/15] fix: a role needs the product installed, not the files it left behind Detection walked a ladder of markers and returned at the first one that fired, with every rung weighted the same. The last PBS rung is a directory, so on a host where proxmox-backup-server had been removed the verdict was dual: /etc/proxmox-backup and /var/lib/proxmox-backup are made at runtime, are owned by no package (dpkg -S finds nothing for either on PBS 3.4.9 or 4.2.0) and the server postrm leaves them alone even on purge. Everything the package ships had gone; what PBS itself created stayed, and that was what decided. The recipe then ran pbs_runtime_core, which treats proxmox-backup-manager as critical, and the whole collection aborted on a command the host never had. The PVE payload was already collected at that point and went out with the workspace, so the host had no backup at all (issue #315). A marker now either proves an install or it does not, and only the first kind ends a ladder. Proving an install means the command answering, a dpkg stanza, a binary the package ships, a package-owned share directory, or a version file with a version in it. The rest are residue: a directory no package owns, an empty version file, and an apt source, which was never evidence of an install and matches the pbs-client repository Proxmox tells you to add on hosts that are not PBS. Residue is recorded rather than dropped. It is the reason an operator expected the other verdict, so the rungs are still walked, the trace marks them as residue instead of a miss, EnvironmentInfo carries the first one per product, and a run reports it at warning: a debug-only line does not reach the person reading an unexpected type. When nothing at all is detected, the error now separates "residue but no install" from "no Proxmox here", because under SYSTEM_ROOT_PREFIX the first usually means the mount carries /etc but not the /usr and /var that hold the package evidence. TestDetectionProvenanceNamesDecidingMarker asserted the old verdict and now asserts this one. The "via sources" and "via directories" subtests did the same for each half of the ladder and are inverted with them. Verified on pve-test (PVE 9.1.9 with PBS 4.2.0 coinstalled), which still reports dual with both halves decided by their command and no residue. --- cmd/proxsave/main_runtime.go | 30 +++ internal/environment/detect.go | 211 ++++++++++++------ .../environment/detect_additional_test.go | 4 +- .../environment/detect_deterministic_test.go | 98 ++++---- .../environment/detect_provenance_test.go | 35 ++- internal/whatsnew/registry.go | 12 + 6 files changed, 272 insertions(+), 118 deletions(-) diff --git a/cmd/proxsave/main_runtime.go b/cmd/proxsave/main_runtime.go index 0562fb60..e3f3d554 100644 --- a/cmd/proxsave/main_runtime.go +++ b/cmd/proxsave/main_runtime.go @@ -67,6 +67,36 @@ func logDetectionProvenance(bootstrap *logging.BootstrapLogger, info *environmen for _, step := range info.Steps { bootstrap.Debug("Detection probe: %s", step) } + warnDetectionResidue(bootstrap, info) +} + +// warnDetectionResidue reports, at warning, a product that left files behind without +// being installed. The trace above says the same thing, but only on a debug run, and +// the operator who needs this is the one looking at a type they did not expect: they +// see PVE where a PBS directory exists, and nothing at a normal level tells them that +// ProxSave looked at that directory and decided it does not count (issue #315). +// +// It stays quiet on a host that has the product, because the ladder returns at the +// first marker that proves an install and never reaches the residue rungs, so there +// is nothing to report on a healthy host of either kind. +func warnDetectionResidue(bootstrap *logging.BootstrapLogger, info *environment.EnvironmentInfo) { + if bootstrap == nil || info == nil { + return + } + for _, residue := range []struct { + product string + marker string + pkg string + }{ + {"PVE", info.PVEResidual, "pve-manager"}, + {"PBS", info.PBSResidual, "proxmox-backup-server"}, + } { + if strings.TrimSpace(residue.marker) == "" { + continue + } + bootstrap.Warning("%s files without a %s install - %s is present but the %s package is not, so this host is collected as %s", + residue.product, residue.product, residue.marker, residue.pkg, info.Type) + } } // detectionSourceLabel keeps the verdict line readable when a product was not diff --git a/internal/environment/detect.go b/internal/environment/detect.go index 432b9a58..1ac5f6f2 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -142,12 +142,12 @@ func GetVersion(pType types.ProxmoxType) (string, error) { switch pType { case types.ProxmoxVE: - if version, ok := detectPVE(nil); ok && version != "" && version != "unknown" { + if version, ok, _ := detectPVE(nil); ok && version != "" && version != "unknown" { return version, nil } return "", fmt.Errorf("unable to determine Proxmox VE version") case types.ProxmoxBS: - if version, ok := detectPBS(nil); ok && version != "" && version != "unknown" { + if version, ok, _ := detectPBS(nil); ok && version != "" && version != "unknown" { return version, nil } return "", fmt.Errorf("unable to determine Proxmox Backup Server version") @@ -251,6 +251,14 @@ func (t *detectionTrace) miss(product, marker, target string) { t.add(DetectionStep{Product: product, Marker: marker, Target: target}) } +// residue records a marker that was found and deliberately does not decide. It is +// not a miss: a miss says the host has nothing there, and this says the host has +// something there that does not answer the question. Both readings matter to whoever +// is holding an unexpected verdict, so they are recorded apart. +func (t *detectionTrace) residue(product, marker, target, note string) { + t.add(DetectionStep{Product: product, Marker: marker, Target: target, Residual: true, Note: note}) +} + func (t *detectionTrace) skip(product, marker, target, note string) { t.add(DetectionStep{Product: product, Marker: marker, Target: target, Skipped: true, Note: note}) } @@ -349,16 +357,18 @@ func detectEnvironmentInfo() (*EnvironmentInfo, error) { extendPath() trace := &detectionTrace{} - pveVersion, hasPVE := detectPVE(trace) - pbsVersion, hasPBS := detectPBS(trace) + pveVersion, hasPVE, pveResidue := detectPVE(trace) + pbsVersion, hasPBS, pbsResidue := detectPBS(trace) info := &EnvironmentInfo{ - Type: resolveType(hasPVE, hasPBS), - PVEVersion: normalizedDetectedVersion(pveVersion), - PBSVersion: normalizedDetectedVersion(pbsVersion), - PVESource: trace.decidedBy(productPVE), - PBSSource: trace.decidedBy(productPBS), - Steps: trace.steps, + Type: resolveType(hasPVE, hasPBS), + PVEVersion: normalizedDetectedVersion(pveVersion), + PBSVersion: normalizedDetectedVersion(pbsVersion), + PVESource: trace.decidedBy(productPVE), + PBSSource: trace.decidedBy(productPBS), + Steps: trace.steps, + PVEResidual: pveResidue, + PBSResidual: pbsResidue, } info.Version = combineVersions(info.PVEVersion, info.PBSVersion) @@ -366,11 +376,32 @@ func detectEnvironmentInfo() (*EnvironmentInfo, error) { return info, nil } - debugPath := writeDetectionDebug() - if debugPath != "" { - return info, fmt.Errorf("unable to detect Proxmox environment (debug saved to %s)", debugPath) + // A host with residue and no install is a different failure from a host with + // nothing on it, and the difference is actionable: under SYSTEM_ROOT_PREFIX it + // usually means the mount carries /etc but not the /usr and /var that hold the + // package evidence. Saying only "unable to detect" would send the operator + // looking for the wrong thing. + if residue := firstNonEmpty(info.PVEResidual, info.PBSResidual); residue != "" { + return info, fmt.Errorf("unable to detect Proxmox environment: %s was found but no installed product was%s", + residue, debugSuffix(writeDetectionDebug())) } - return info, fmt.Errorf("unable to detect Proxmox environment") + return info, fmt.Errorf("unable to detect Proxmox environment%s", debugSuffix(writeDetectionDebug())) +} + +func firstNonEmpty(values ...string) string { + for _, value := range values { + if strings.TrimSpace(value) != "" { + return value + } + } + return "" +} + +func debugSuffix(path string) string { + if path == "" { + return "" + } + return " (debug saved to " + path + ")" } func resolveType(hasPVE, hasPBS bool) types.ProxmoxType { @@ -407,118 +438,157 @@ func combineVersions(pveVersion, pbsVersion string) string { } } -// detectPVE walks the PVE marker ladder and returns at the first marker that fires. -// Every rung it reaches is recorded in trace (nil records nothing), so a log can say -// which marker produced the verdict and which ones it had already ruled out. -func detectPVE(trace *detectionTrace) (string, bool) { +// detectPVE walks the PVE marker ladder and returns at the first marker that PROVES +// the product is installed. Every rung it reaches is recorded in trace (nil records +// nothing), so a log can say which marker produced the verdict and which ones it had +// already ruled out. The third return is the first residue seen: see detectPBS, which +// is where that distinction is load-bearing. +func detectPVE(trace *detectionTrace) (string, bool, string) { + residue := "" + noteResidue := func(marker, target, note string) { + trace.residue(productPVE, marker, target, note) + if residue == "" { + residue = fmt.Sprintf("%s (%s)", marker, target) + } + } + if hostRooted() { trace.skip(productPVE, "command", "pveversion", "a command run here answers for the appliance, not for the mounted host") } else if version, ok := detectPVEViaCommand(); ok { trace.hit(productPVE, "command", "pveversion", version) - return version, true + return version, true, "" } else { trace.miss(productPVE, "command", "pveversion") } versionFiles := targetList(pveVersionFile, pveLegacyFile) - if version, ok := detectPVEViaVersionFiles(); ok { + switch version, outcome := detectPVEViaVersionFiles(); outcome { + case markerInstalled: trace.hit(productPVE, "version-file", versionFiles, version) - return version, true + return version, true, "" + case markerResidual: + noteResidue("version-file", versionFiles, "the file is there but carries no version") + default: + trace.miss(productPVE, "version-file", versionFiles) } - trace.miss(productPVE, "version-file", versionFiles) // dpkg is version-bearing and reliable offline, so it precedes the version-less // markers below and recovers the real version even when the pmxcfs version files // are absent. if version, ok := dpkgPackageInstalled("pve-manager"); ok { trace.hit(productPVE, "dpkg pve-manager", resolveUnderPrefix(dpkgStatusFile), version) - return version, true + return version, true, "" } trace.miss(productPVE, "dpkg pve-manager", resolveUnderPrefix(dpkgStatusFile)) if fileExists(pveClusterDB) { trace.hit(productPVE, "cluster-db", resolveUnderPrefix(pveClusterDB), "") - return "unknown", true + return "unknown", true, "" } trace.miss(productPVE, "cluster-db", resolveUnderPrefix(pveClusterDB)) if path := firstExistingFile(pveBinaryCandidates); path != "" { trace.hit(productPVE, "binary", path, "") - return "unknown", true + return "unknown", true, "" } trace.miss(productPVE, "binary", targetList(pveBinaryCandidates...)) if dirExists(pveShareDir) { trace.hit(productPVE, "share-dir", resolveUnderPrefix(pveShareDir), "") - return "unknown", true + return "unknown", true, "" } trace.miss(productPVE, "share-dir", resolveUnderPrefix(pveShareDir)) if path := firstMatchingSource(pveSourceFiles, pveSourceTokens); path != "" { - trace.hit(productPVE, "apt-source", path, "") - return "unknown", true + noteResidue("apt-source", path, "a configured repository is not an installed package") + } else { + trace.miss(productPVE, "apt-source", targetList(pveSourceFiles...)) } - trace.miss(productPVE, "apt-source", targetList(pveSourceFiles...)) if path := firstExistingDir(pveDirCandidates); path != "" { - trace.hit(productPVE, "directory", path, "") - return "unknown", true + noteResidue("directory", path, "no package owns this directory and none removes it") + } else { + trace.miss(productPVE, "directory", targetList(pveDirCandidates...)) } - trace.miss(productPVE, "directory", targetList(pveDirCandidates...)) - return "", false + return "", false, residue } -// detectPBS is the PBS half of the same ladder, traced the same way. The version-less -// rungs here are the ones that turn a PVE-only host into a dual verdict when a PBS -// install left something behind, which is why each records the exact path it matched. -func detectPBS(trace *detectionTrace) (string, bool) { +// detectPBS is the PBS half of the same ladder, traced the same way, and it is the +// half where the split between an install and its leftovers was bought with a broken +// backup (issue #315). +// +// Everything proxmox-backup-server SHIPS goes away when the package does: the manager +// binary, /usr/share/proxmox-backup (dpkg -S names the package as its owner) and the +// dpkg stanza itself. Everything PBS CREATES stays: /etc/proxmox-backup and +// /var/lib/proxmox-backup belong to no package, are made at runtime, and the server +// postrm does not remove them even on purge. Measured on PBS 3.4.9 and 4.2.0. +// +// So the two kinds of marker answer different questions. The shipped ones answer "is +// PBS installed"; the created ones answer "was PBS ever here", which is not the +// question the backup recipe needs. Only the first kind ends this ladder. +func detectPBS(trace *detectionTrace) (string, bool, string) { + residue := "" + noteResidue := func(marker, target, note string) { + trace.residue(productPBS, marker, target, note) + if residue == "" { + residue = fmt.Sprintf("%s (%s)", marker, target) + } + } + if hostRooted() { trace.skip(productPBS, "command", "proxmox-backup-manager", "a command run here answers for the appliance, not for the mounted host") } else if version, ok := detectPBSViaCommand(); ok { trace.hit(productPBS, "command", "proxmox-backup-manager", version) - return version, true + return version, true, "" } else { trace.miss(productPBS, "command", "proxmox-backup-manager") } - if version, ok := detectPBSViaVersionFile(); ok { + switch version, outcome := detectPBSViaVersionFile(); outcome { + case markerInstalled: trace.hit(productPBS, "version-file", resolveUnderPrefix(pbsVersionFile), version) - return version, true + return version, true, "" + case markerResidual: + noteResidue("version-file", resolveUnderPrefix(pbsVersionFile), "the file is there but carries no version") + default: + trace.miss(productPBS, "version-file", resolveUnderPrefix(pbsVersionFile)) } - trace.miss(productPBS, "version-file", resolveUnderPrefix(pbsVersionFile)) if version, ok := dpkgPackageInstalled("proxmox-backup-server"); ok { trace.hit(productPBS, "dpkg proxmox-backup-server", resolveUnderPrefix(dpkgStatusFile), version) - return version, true + return version, true, "" } trace.miss(productPBS, "dpkg proxmox-backup-server", resolveUnderPrefix(dpkgStatusFile)) if path := firstExistingFile(pbsBinaryCandidates); path != "" { trace.hit(productPBS, "binary", path, "") - return "unknown", true + return "unknown", true, "" } trace.miss(productPBS, "binary", targetList(pbsBinaryCandidates...)) if dirExists(pbsShareDir) { trace.hit(productPBS, "share-dir", resolveUnderPrefix(pbsShareDir), "") - return "unknown", true + return "unknown", true, "" } trace.miss(productPBS, "share-dir", resolveUnderPrefix(pbsShareDir)) + // The apt candidates carry the token "pbs", and the repository Proxmox tells you to + // add on a non-PBS host to keep proxmox-backup-client current is called pbs-client. + // A configured repository never proved an install; on this rung it used to. if path := firstMatchingSource(pbsSourceFiles, pbsSourceTokens); path != "" { - trace.hit(productPBS, "apt-source", path, "") - return "unknown", true + noteResidue("apt-source", path, "a configured repository is not an installed package") + } else { + trace.miss(productPBS, "apt-source", targetList(pbsSourceFiles...)) } - trace.miss(productPBS, "apt-source", targetList(pbsSourceFiles...)) if path := firstExistingDir(pbsDirCandidates); path != "" { - trace.hit(productPBS, "directory", path, "") - return "unknown", true + noteResidue("directory", path, "no package owns this directory and none removes it") + } else { + trace.miss(productPBS, "directory", targetList(pbsDirCandidates...)) } - trace.miss(productPBS, "directory", targetList(pbsDirCandidates...)) - return "", false + return "", false, residue } // firstExistingFile returns the first candidate that resolves to a regular file @@ -628,32 +698,47 @@ func detectPBSViaCommand() (string, bool) { return version, true } -func detectPVEViaVersionFiles() (string, bool) { +// markerOutcome is what a version-file probe found. The middle state is the point: +// a file that exists but says nothing is neither "the product is here" nor "there is +// nothing here", and collapsing it onto either one loses the only detail that +// explains the verdict. +type markerOutcome int + +const ( + markerAbsent markerOutcome = iota + markerResidual + markerInstalled +) + +func detectPVEViaVersionFiles() (string, markerOutcome) { + outcome := markerAbsent + if fileExists(pveVersionFile) { if version := readAndTrim(pveVersionFile); version != "" { - return version, true + return version, markerInstalled } + outcome = markerResidual } if fileExists(pveLegacyFile) { data := readAndTrim(pveLegacyFile) if version := extractPVEVersion(data); version != "" { - return version, true + return version, markerInstalled } - return "unknown", true + outcome = markerResidual } - return "", false + return "", outcome } -func detectPBSViaVersionFile() (string, bool) { - if fileExists(pbsVersionFile) { - if version := readAndTrim(pbsVersionFile); version != "" { - return version, true - } - return "unknown", true +func detectPBSViaVersionFile() (string, markerOutcome) { + if !fileExists(pbsVersionFile) { + return "", markerAbsent } - return "", false + if version := readAndTrim(pbsVersionFile); version != "" { + return version, markerInstalled + } + return "", markerResidual } func detectPVEViaSources() bool { diff --git a/internal/environment/detect_additional_test.go b/internal/environment/detect_additional_test.go index a84f9ab5..93c3cc8b 100644 --- a/internal/environment/detect_additional_test.go +++ b/internal/environment/detect_additional_test.go @@ -451,7 +451,7 @@ func TestDetectPBSViaCommand(t *testing.T) { // TestDetectPVE tests complete PVE detection func TestDetectPVE(t *testing.T) { - version, ok := detectPVE(nil) + version, ok, _ := detectPVE(nil) // On non-PVE systems, should return false // On PVE systems, should return true with version @@ -462,7 +462,7 @@ func TestDetectPVE(t *testing.T) { // TestDetectPBS tests complete PBS detection func TestDetectPBS(t *testing.T) { - version, ok := detectPBS(nil) + version, ok, _ := detectPBS(nil) // On non-PBS systems, should return false // On PBS systems, should return true with version diff --git a/internal/environment/detect_deterministic_test.go b/internal/environment/detect_deterministic_test.go index 118835f9..779b4d14 100644 --- a/internal/environment/detect_deterministic_test.go +++ b/internal/environment/detect_deterministic_test.go @@ -159,9 +159,9 @@ func TestDetectPVEViaVersionFiles_Branches(t *testing.T) { setValue(t, &pveVersionFile, versionFile) setValue(t, &pveLegacyFile, filepath.Join(tmpDir, "missing-legacy")) - version, ok := detectPVEViaVersionFiles() - if !ok || version != "7.4-1" { - t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, ok, "7.4-1", true) + version, outcome := detectPVEViaVersionFiles() + if outcome != markerInstalled || version != "7.4-1" { + t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, outcome, "7.4-1", markerInstalled) } }) @@ -178,9 +178,9 @@ func TestDetectPVEViaVersionFiles_Branches(t *testing.T) { setValue(t, &pveVersionFile, versionFile) setValue(t, &pveLegacyFile, legacyFile) - version, ok := detectPVEViaVersionFiles() - if !ok || version != "7.4-3" { - t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, ok, "7.4-3", true) + version, outcome := detectPVEViaVersionFiles() + if outcome != markerInstalled || version != "7.4-3" { + t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, outcome, "7.4-3", markerInstalled) } }) @@ -193,9 +193,9 @@ func TestDetectPVEViaVersionFiles_Branches(t *testing.T) { setValue(t, &pveVersionFile, filepath.Join(tmpDir, "missing-version")) setValue(t, &pveLegacyFile, legacyFile) - version, ok := detectPVEViaVersionFiles() - if !ok || version != "unknown" { - t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, outcome := detectPVEViaVersionFiles() + if outcome != markerResidual || version != "" { + t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v): a file with no version in it proves nothing", version, outcome, "", markerResidual) } }) @@ -203,9 +203,9 @@ func TestDetectPVEViaVersionFiles_Branches(t *testing.T) { setValue(t, &pveVersionFile, filepath.Join(tmpDir, "missing-version-2")) setValue(t, &pveLegacyFile, filepath.Join(tmpDir, "missing-legacy-2")) - version, ok := detectPVEViaVersionFiles() - if ok || version != "" { - t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, ok, "", false) + version, outcome := detectPVEViaVersionFiles() + if outcome != markerAbsent || version != "" { + t.Fatalf("detectPVEViaVersionFiles() = (%q, %v), want (%q, %v)", version, outcome, "", markerAbsent) } }) } @@ -221,9 +221,9 @@ func TestDetectPBSViaVersionFile_Branches(t *testing.T) { setValue(t, &pbsVersionFile, versionFile) - version, ok := detectPBSViaVersionFile() - if !ok || version != "2.4-1" { - t.Fatalf("detectPBSViaVersionFile() = (%q, %v), want (%q, %v)", version, ok, "2.4-1", true) + version, outcome := detectPBSViaVersionFile() + if outcome != markerInstalled || version != "2.4-1" { + t.Fatalf("detectPBSViaVersionFile() = (%q, %v), want (%q, %v)", version, outcome, "2.4-1", markerInstalled) } }) @@ -235,18 +235,18 @@ func TestDetectPBSViaVersionFile_Branches(t *testing.T) { setValue(t, &pbsVersionFile, versionFile) - version, ok := detectPBSViaVersionFile() - if !ok || version != "unknown" { - t.Fatalf("detectPBSViaVersionFile() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, outcome := detectPBSViaVersionFile() + if outcome != markerResidual || version != "" { + t.Fatalf("detectPBSViaVersionFile() = (%q, %v), want (%q, %v): an empty file proves nothing", version, outcome, "", markerResidual) } }) t.Run("missing", func(t *testing.T) { setValue(t, &pbsVersionFile, filepath.Join(tmpDir, "missing-pbs-version")) - version, ok := detectPBSViaVersionFile() - if ok || version != "" { - t.Fatalf("detectPBSViaVersionFile() = (%q, %v), want (%q, %v)", version, ok, "", false) + version, outcome := detectPBSViaVersionFile() + if outcome != markerAbsent || version != "" { + t.Fatalf("detectPBSViaVersionFile() = (%q, %v), want (%q, %v)", version, outcome, "", markerAbsent) } }) } @@ -299,7 +299,7 @@ func TestDetectPVE_FallbackOrder(t *testing.T) { return "pve-manager/7.4-3/d4a3b4a1", nil }) - version, ok := detectPVE(nil) + version, ok, _ := detectPVE(nil) if !ok || version != "7.4-3" { t.Fatalf("detectPVE() = (%q, %v), want (%q, %v)", version, ok, "7.4-3", true) } @@ -315,13 +315,13 @@ func TestDetectPVE_FallbackOrder(t *testing.T) { setValue(t, &pveVersionFile, versionFile) setValue(t, &pveLegacyFile, filepath.Join(tmpDir, "missing-legacy")) - version, ok := detectPVE(nil) + version, ok, _ := detectPVE(nil) if !ok || version != "7.4-1" { t.Fatalf("detectPVE() = (%q, %v), want (%q, %v)", version, ok, "7.4-1", true) } }) - t.Run("via sources", func(t *testing.T) { + t.Run("an apt source is residue, not a verdict", func(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "", errors.New("not found") }) setValue(t, &pveVersionFile, filepath.Join(tmpDir, "missing-version")) setValue(t, &pveLegacyFile, filepath.Join(tmpDir, "missing-legacy")) @@ -332,13 +332,16 @@ func TestDetectPVE_FallbackOrder(t *testing.T) { } setValue(t, &pveSourceFiles, []string{sourceFile}) - version, ok := detectPVE(nil) - if !ok || version != "unknown" { - t.Fatalf("detectPVE() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, ok, residue := detectPVE(nil) + if ok || version != "" { + t.Fatalf("detectPVE() = (%q, %v), want it not to decide: a configured repository is not an installed package", version, ok) + } + if !strings.Contains(residue, "apt-source") { + t.Fatalf("residue = %q, want the apt source named", residue) } }) - t.Run("via directories", func(t *testing.T) { + t.Run("a leftover directory is residue, not a verdict", func(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "", errors.New("not found") }) setValue(t, &pveVersionFile, filepath.Join(tmpDir, "missing-version-2")) setValue(t, &pveLegacyFile, filepath.Join(tmpDir, "missing-legacy-2")) @@ -350,9 +353,12 @@ func TestDetectPVE_FallbackOrder(t *testing.T) { } setValue(t, &pveDirCandidates, []string{dirCandidate}) - version, ok := detectPVE(nil) - if !ok || version != "unknown" { - t.Fatalf("detectPVE() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, ok, residue := detectPVE(nil) + if ok || version != "" { + t.Fatalf("detectPVE() = (%q, %v), want it not to decide: no package owns this directory", version, ok) + } + if !strings.Contains(residue, dirCandidate) { + t.Fatalf("residue = %q, want it to name %s", residue, dirCandidate) } }) @@ -363,7 +369,7 @@ func TestDetectPVE_FallbackOrder(t *testing.T) { setValue(t, &pveSourceFiles, []string{}) setValue(t, &pveDirCandidates, []string{}) - version, ok := detectPVE(nil) + version, ok, _ := detectPVE(nil) if ok || version != "" { t.Fatalf("detectPVE() = (%q, %v), want (%q, %v)", version, ok, "", false) } @@ -381,7 +387,7 @@ func TestDetectPBS_FallbackOrder(t *testing.T) { return "version: 2.4.1", nil }) - version, ok := detectPBS(nil) + version, ok, _ := detectPBS(nil) if !ok || version != "2.4.1" { t.Fatalf("detectPBS() = (%q, %v), want (%q, %v)", version, ok, "2.4.1", true) } @@ -396,13 +402,13 @@ func TestDetectPBS_FallbackOrder(t *testing.T) { } setValue(t, &pbsVersionFile, versionFile) - version, ok := detectPBS(nil) + version, ok, _ := detectPBS(nil) if !ok || version != "2.4-1" { t.Fatalf("detectPBS() = (%q, %v), want (%q, %v)", version, ok, "2.4-1", true) } }) - t.Run("via sources", func(t *testing.T) { + t.Run("an apt source is residue, not a verdict", func(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "", errors.New("not found") }) setValue(t, &pbsVersionFile, filepath.Join(tmpDir, "missing-pbs-version")) @@ -412,13 +418,16 @@ func TestDetectPBS_FallbackOrder(t *testing.T) { } setValue(t, &pbsSourceFiles, []string{sourceFile}) - version, ok := detectPBS(nil) - if !ok || version != "unknown" { - t.Fatalf("detectPBS() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, ok, residue := detectPBS(nil) + if ok || version != "" { + t.Fatalf("detectPBS() = (%q, %v), want it not to decide: a configured repository is not an installed package", version, ok) + } + if !strings.Contains(residue, "apt-source") { + t.Fatalf("residue = %q, want the apt source named", residue) } }) - t.Run("via directories", func(t *testing.T) { + t.Run("a leftover directory is residue, not a verdict", func(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "", errors.New("not found") }) setValue(t, &pbsVersionFile, filepath.Join(tmpDir, "missing-pbs-version-2")) setValue(t, &pbsSourceFiles, []string{}) @@ -429,9 +438,12 @@ func TestDetectPBS_FallbackOrder(t *testing.T) { } setValue(t, &pbsDirCandidates, []string{dirCandidate}) - version, ok := detectPBS(nil) - if !ok || version != "unknown" { - t.Fatalf("detectPBS() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, ok, residue := detectPBS(nil) + if ok || version != "" { + t.Fatalf("detectPBS() = (%q, %v), want it not to decide: no package owns this directory", version, ok) + } + if !strings.Contains(residue, dirCandidate) { + t.Fatalf("residue = %q, want it to name %s", residue, dirCandidate) } }) @@ -441,7 +453,7 @@ func TestDetectPBS_FallbackOrder(t *testing.T) { setValue(t, &pbsSourceFiles, []string{}) setValue(t, &pbsDirCandidates, []string{}) - version, ok := detectPBS(nil) + version, ok, _ := detectPBS(nil) if ok || version != "" { t.Fatalf("detectPBS() = (%q, %v), want (%q, %v)", version, ok, "", false) } diff --git a/internal/environment/detect_provenance_test.go b/internal/environment/detect_provenance_test.go index 60ce32d0..41243199 100644 --- a/internal/environment/detect_provenance_test.go +++ b/internal/environment/detect_provenance_test.go @@ -19,14 +19,19 @@ func stepFor(steps []DetectionStep, product, marker string) (DetectionStep, bool return DetectionStep{}, false } -// TestDetectionProvenanceNamesDecidingMarker is the issue #315 case: a host whose -// only PBS marker is a leftover directory is reported as dual, and the trace has to -// say so - naming the directory that decided it and showing that every version-bearing -// PBS marker missed. +// TestDetectionProvenanceNamesDecidingMarker is the issue #315 case, and this test +// used to assert the bug: a host whose only PBS marker is a leftover directory was +// reported as dual. It is now the record of what the host deserves. PVE decides on a +// version-bearing marker and PBS decides on nothing, because a directory no package +// owns survives the package and so cannot stand for it. +// +// The leftover is still reported. It is the reason the operator expected PBS, so the +// trace names it as residue and PBSSource stays empty: nothing decided that half. func TestDetectionProvenanceNamesDecidingMarker(t *testing.T) { root := t.TempDir() writeFile(t, filepath.Join(root, "etc/pve-manager/version"), "8.2.2\n") - if err := os.MkdirAll(filepath.Join(root, "var/lib/proxmox-backup"), 0o755); err != nil { + leftover := filepath.Join(root, "var/lib/proxmox-backup") + if err := os.MkdirAll(leftover, 0o755); err != nil { t.Fatal(err) } @@ -34,16 +39,26 @@ func TestDetectionProvenanceNamesDecidingMarker(t *testing.T) { if err != nil { t.Fatalf("DetectWith: %v", err) } - if info.Type != types.ProxmoxDual { - t.Fatalf("Type = %v, want ProxmoxDual", info.Type) + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) } if !strings.HasPrefix(info.PVESource, "version-file (") { t.Fatalf("PVESource = %q, want the version-file marker", info.PVESource) } - wantDir := filepath.Join(root, "var/lib/proxmox-backup") - if !strings.Contains(info.PBSSource, wantDir) { - t.Fatalf("PBSSource = %q, want the deciding directory %s", info.PBSSource, wantDir) + if info.PBSSource != "" { + t.Fatalf("PBSSource = %q, want no marker to have decided PBS", info.PBSSource) + } + if !strings.Contains(info.PBSResidual, leftover) { + t.Fatalf("PBSResidual = %q, want it to name %s", info.PBSResidual, leftover) + } + + dirStep, ok := stepFor(info.Steps, productPBS, "directory") + if !ok { + t.Fatal("no PBS directory step recorded: the rung is still walked, it just does not decide") + } + if dirStep.Hit || !dirStep.Residual { + t.Fatalf("PBS directory step = %v, want residue", dirStep) } versionStep, ok := stepFor(info.Steps, productPBS, "version-file") diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index b81b18d9..04115963 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -201,6 +201,18 @@ var notes = []Note{ "If SKIP_PERMISSION_CHECK=true is left over from a test, clear it: the next run really skips that check", }, }, + { + Version: "0.39.0", + Lines: []string{ + "A PVE host with leftover proxmox-backup files is no longer taken for a PBS server, and its backup runs again", + "A run says when a product left files behind without being installed, and names the file it found", + }, + Actions: []string{ + "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", + "The leftover directories are still reported: remove them, or install PBS, only if you want the notice to stop", + "An archive taken before this fix is labelled dual, so restoring it on the corrected host reports partial match", + }, + }, } // LookupNotes returns the notes for versions in the half-open range (from, to], ascending by From 1a8d77533349fe4e03b7b42131b3729410d3de86 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:32:11 +0200 Subject: [PATCH 03/15] fix: restore reads the host type from the same check the backup uses DetectCurrentSystem carried its own rule, and the rule had rotted. hasPBS was `/etc/proxmox-backup` OR `/usr/sbin/proxmox-backup-proxy`, and that second path does not exist on PBS 3.4.9 or on 4.2.0: the proxy is a systemd unit, not a binary on PATH. So the OR was never a choice between two proofs. Every PBS decision on the restore side rested on one directory, the same directory that turned a PVE-only host into a dual backup in issue #315, and it rested on it with no ladder behind it: no dpkg stanza, no version, no package-owned file. That mattered beyond the label. SupportsPBS() gates the staged PBS apply, the PBS access-control and notification paths, datastore directory recreation, the mount guard, and NeedsPBSServices, which asks systemd to stop proxmox-backup-proxy and proxmox-backup. The common `ssl` category lists ./etc/proxmox-backup/proxy.pem among its paths and shouldStopPBSServices reads the category definition rather than the archive, so selecting SSL on a host with leftovers was enough to send the restore stopping services that host does not have. It now delegates to environment.Detect, so a host cannot be one type while being collected and another while being restored onto. detectEnvironment is the seam for tests, mirroring compatFS. The three table cases built their verdict by creating directories on the fake filesystem, which is exactly the rule being removed; they now stub the verdict and assert the mapping, and the rule itself stays covered where it lives. Two cases are added for what this changes: a host whose only PBS evidence is residue restores as PVE, and a nil verdict fails closed to unknown rather than to PVE. --- internal/orchestrator/compatibility.go | 41 +++--- internal/orchestrator/compatibility_test.go | 117 ++++++++---------- internal/orchestrator/deps_additional_test.go | 18 +-- internal/whatsnew/registry.go | 2 + 4 files changed, 89 insertions(+), 89 deletions(-) diff --git a/internal/orchestrator/compatibility.go b/internal/orchestrator/compatibility.go index 785d8d99..d14736ca 100644 --- a/internal/orchestrator/compatibility.go +++ b/internal/orchestrator/compatibility.go @@ -5,6 +5,8 @@ import ( "strings" "github.com/tis24dev/proxsave/internal/backup" + "github.com/tis24dev/proxsave/internal/environment" + "github.com/tis24dev/proxsave/internal/types" ) var compatFS FS = osFS{} @@ -42,25 +44,36 @@ func (s SystemType) Overlaps(other SystemType) bool { return (s.SupportsPVE() && other.SupportsPVE()) || (s.SupportsPBS() && other.SupportsPBS()) } -// DetectCurrentSystem detects the type of the current system (PVE or PBS) +// detectEnvironment is the seam that lets a test drive DetectCurrentSystem without a +// Proxmox host, the way compatFS does for the file probes in this file. +var detectEnvironment = environment.Detect + +// DetectCurrentSystem reports what this host is, for the restore side. +// +// It used to carry its own rule, and the rule had rotted: hasPBS was +// `/etc/proxmox-backup` OR `/usr/sbin/proxmox-backup-proxy`, and the second path does +// not exist on PBS 3.4.9 or on 4.2.0 (the proxy is a systemd unit, not a binary on +// PATH). So the OR was never a choice between two proofs. Every PBS restore decision +// rested on one directory, which no package owns and no removal deletes: the same +// marker that turned a PVE-only host into a dual backup in issue #315. +// +// Backup and restore now read the same ladder, so a host cannot be one type while +// being collected and another while being restored onto. func DetectCurrentSystem() SystemType { - hasPVE := fileExists("/etc/pve") || fileExists("/usr/bin/qm") || fileExists("/usr/bin/pct") - hasPBS := fileExists("/etc/proxmox-backup") || fileExists("/usr/sbin/proxmox-backup-proxy") - - // Check for PVE indicators - if hasPVE && hasPBS { - return SystemTypeDual + info, _ := detectEnvironment() + if info == nil { + return SystemTypeUnknown } - if hasPVE { + switch info.Type { + case types.ProxmoxDual: + return SystemTypeDual + case types.ProxmoxVE: return SystemTypePVE - } - - // Check for PBS indicators - if hasPBS { + case types.ProxmoxBS: return SystemTypePBS + default: + return SystemTypeUnknown } - - return SystemTypeUnknown } // DetectBackupType detects the type of backup from manifest diff --git a/internal/orchestrator/compatibility_test.go b/internal/orchestrator/compatibility_test.go index 5572496e..1cef099c 100644 --- a/internal/orchestrator/compatibility_test.go +++ b/internal/orchestrator/compatibility_test.go @@ -1,12 +1,15 @@ package orchestrator import ( + "errors" "os" "reflect" "strings" "testing" "github.com/tis24dev/proxsave/internal/backup" + "github.com/tis24dev/proxsave/internal/environment" + "github.com/tis24dev/proxsave/internal/types" ) func TestSystemTypeCapabilities(t *testing.T) { @@ -155,67 +158,64 @@ func TestValidateCompatibility_Branches(t *testing.T) { } } +// stubDetection points DetectCurrentSystem at a fixed verdict. The rule itself lives +// in internal/environment and is exercised there; what this file has to prove is that +// restore asks that rule rather than keeping a second one of its own. +func stubDetection(t *testing.T, info *environment.EnvironmentInfo, err error) { + t.Helper() + orig := detectEnvironment + t.Cleanup(func() { detectEnvironment = orig }) + detectEnvironment = func() (*environment.EnvironmentInfo, error) { return info, err } +} + func TestDetectCurrentSystem_Unknown(t *testing.T) { - orig := compatFS - defer func() { compatFS = orig }() - fake := NewFakeFS() - defer func() { _ = os.RemoveAll(fake.Root) }() - compatFS = fake + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxUnknown}, errors.New("nothing detected")) if got := DetectCurrentSystem(); got != SystemTypeUnknown { t.Fatalf("expected unknown system, got %s", got) } } +// TestDetectCurrentSystemIgnoresLeftoverPBSFiles is the restore half of issue #315. +// The rule this replaced read /etc/proxmox-backup directly, so a host that had PBS +// removed was restored onto as if it were still a backup server: PBS categories were +// offered and NeedsPBSServices asked systemd to stop services the host does not have. +func TestDetectCurrentSystemIgnoresLeftoverPBSFiles(t *testing.T) { + stubDetection(t, &environment.EnvironmentInfo{ + Type: types.ProxmoxVE, + PBSResidual: "directory (/etc/proxmox-backup)", + }, nil) + + if got := DetectCurrentSystem(); got != SystemTypePVE { + t.Fatalf("DetectCurrentSystem() = %s, want %s", got, SystemTypePVE) + } +} + +// TestDetectCurrentSystemSurvivesANilVerdict: detection never returns nil today, and +// a nil here must not become a PVE restore by accident. +func TestDetectCurrentSystemSurvivesANilVerdict(t *testing.T) { + stubDetection(t, nil, errors.New("boom")) + + if got := DetectCurrentSystem(); got != SystemTypeUnknown { + t.Fatalf("DetectCurrentSystem() = %s, want %s", got, SystemTypeUnknown) + } +} + func TestDetectCurrentSystem_Branches(t *testing.T) { tests := []struct { - name string - setup func(t *testing.T, fake *FakeFS) - want SystemType + name string + detected types.ProxmoxType + want SystemType }{ - { - name: "pve only", - setup: func(t *testing.T, fake *FakeFS) { - t.Helper() - if err := fake.AddDir("/etc/pve"); err != nil { - t.Fatalf("add dir: %v", err) - } - }, - want: SystemTypePVE, - }, - { - name: "pbs only", - setup: func(t *testing.T, fake *FakeFS) { - t.Helper() - if err := fake.AddDir("/etc/proxmox-backup"); err != nil { - t.Fatalf("add dir: %v", err) - } - }, - want: SystemTypePBS, - }, - { - name: "dual", - setup: func(t *testing.T, fake *FakeFS) { - t.Helper() - if err := fake.AddDir("/etc/pve"); err != nil { - t.Fatalf("add pve dir: %v", err) - } - if err := fake.AddDir("/etc/proxmox-backup"); err != nil { - t.Fatalf("add pbs dir: %v", err) - } - }, - want: SystemTypeDual, - }, + {name: "pve only", detected: types.ProxmoxVE, want: SystemTypePVE}, + {name: "pbs only", detected: types.ProxmoxBS, want: SystemTypePBS}, + {name: "dual", detected: types.ProxmoxDual, want: SystemTypeDual}, + {name: "unknown", detected: types.ProxmoxUnknown, want: SystemTypeUnknown}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - orig := compatFS - defer func() { compatFS = orig }() - fake := NewFakeFS() - defer func() { _ = os.RemoveAll(fake.Root) }() - compatFS = fake - tt.setup(t, fake) + stubDetection(t, &environment.EnvironmentInfo{Type: tt.detected}, nil) if got := DetectCurrentSystem(); got != tt.want { t.Fatalf("DetectCurrentSystem() = %s; want %s", got, tt.want) @@ -318,9 +318,7 @@ func TestGetSystemInfoDetectsPVE(t *testing.T) { fake := NewFakeFS() defer func() { _ = os.RemoveAll(fake.Root) }() compatFS = fake - if err := fake.AddDir("/etc/pve"); err != nil { - t.Fatalf("add dir: %v", err) - } + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxVE}, nil) if err := fake.WriteFile("/etc/pve-release", []byte("Proxmox VE 8.1\n"), 0o644); err != nil { t.Fatalf("write release: %v", err) } @@ -350,9 +348,7 @@ func TestGetSystemInfoDetectsPBS(t *testing.T) { fake := NewFakeFS() defer func() { _ = os.RemoveAll(fake.Root) }() compatFS = fake - if err := fake.AddDir("/etc/proxmox-backup"); err != nil { - t.Fatalf("add dir: %v", err) - } + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxBS}, nil) if err := fake.WriteFile("/etc/proxmox-backup-release", []byte("Proxmox Backup Server 3.0\n"), 0o644); err != nil { t.Fatalf("write release: %v", err) } @@ -380,12 +376,7 @@ func TestGetSystemInfo_DualAndUnknown(t *testing.T) { fake := NewFakeFS() defer func() { _ = os.RemoveAll(fake.Root) }() compatFS = fake - if err := fake.AddDir("/etc/pve"); err != nil { - t.Fatalf("add pve dir: %v", err) - } - if err := fake.AddDir("/etc/proxmox-backup"); err != nil { - t.Fatalf("add pbs dir: %v", err) - } + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxDual}, nil) if err := fake.WriteFile("/etc/pve-release", []byte("Proxmox VE 8.2\n"), 0o644); err != nil { t.Fatalf("write pve release: %v", err) } @@ -418,6 +409,7 @@ func TestGetSystemInfo_DualAndUnknown(t *testing.T) { fake := NewFakeFS() defer func() { _ = os.RemoveAll(fake.Root) }() compatFS = fake + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxUnknown}, errors.New("nothing detected")) if err := fake.WriteFile("/etc/hostname", []byte("generic-node\n"), 0o644); err != nil { t.Fatalf("write hostname: %v", err) } @@ -446,7 +438,8 @@ func TestCheckSystemRequirements(t *testing.T) { fake := NewFakeFS() defer func() { _ = os.RemoveAll(fake.Root) }() compatFS = fake - for _, dir := range []string{"/etc", "/var", "/usr", "/etc/pve", "/etc/proxmox-backup"} { + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxDual}, nil) + for _, dir := range []string{"/etc", "/var", "/usr"} { if err := fake.AddDir(dir); err != nil { t.Fatalf("add dir %s: %v", dir, err) } @@ -468,9 +461,7 @@ func TestCheckSystemRequirements(t *testing.T) { fake := NewFakeFS() defer func() { _ = os.RemoveAll(fake.Root) }() compatFS = fake - if err := fake.AddDir("/etc/proxmox-backup"); err != nil { - t.Fatalf("add dir: %v", err) - } + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxBS}, nil) fake.StatErr["/"] = os.ErrPermission warnings := CheckSystemRequirements(&backup.Manifest{ProxmoxTargets: []string{"pve"}}) diff --git a/internal/orchestrator/deps_additional_test.go b/internal/orchestrator/deps_additional_test.go index 7b26981a..8f2d97a0 100644 --- a/internal/orchestrator/deps_additional_test.go +++ b/internal/orchestrator/deps_additional_test.go @@ -4,13 +4,13 @@ import ( "context" "io" "os" - "path/filepath" "runtime" "strings" "testing" "time" "github.com/tis24dev/proxsave/internal/config" + "github.com/tis24dev/proxsave/internal/environment" "github.com/tis24dev/proxsave/internal/logging" "github.com/tis24dev/proxsave/internal/types" ) @@ -204,17 +204,11 @@ func TestOSCommandRunner_RunAndRunStream(t *testing.T) { } } -func TestRealSystemDetectorUsesCompatFS(t *testing.T) { - orig := compatFS - t.Cleanup(func() { compatFS = orig }) - - fake := NewFakeFS() - t.Cleanup(func() { _ = os.RemoveAll(fake.Root) }) - compatFS = fake - - if err := fake.AddDir(filepath.Join(string(os.PathSeparator), "etc", "pve")); err != nil { - t.Fatalf("AddDir: %v", err) - } +// TestRealSystemDetectorUsesTheDetectionLadder: the injected detector must reach the +// same rule the backup side uses. It used to read /etc/pve off compatFS, which is how +// restore ended up with a second, weaker opinion about what this host is. +func TestRealSystemDetectorUsesTheDetectionLadder(t *testing.T) { + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxVE}, nil) got := (realSystemDetector{}).DetectCurrentSystem() if got != SystemTypePVE { diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index 04115963..4193dfd2 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -206,11 +206,13 @@ var notes = []Note{ Lines: []string{ "A PVE host with leftover proxmox-backup files is no longer taken for a PBS server, and its backup runs again", "A run says when a product left files behind without being installed, and names the file it found", + "Restore reads the host type from the same check the backup uses, instead of keeping a weaker one of its own", }, Actions: []string{ "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", "The leftover directories are still reported: remove them, or install PBS, only if you want the notice to stop", "An archive taken before this fix is labelled dual, so restoring it on the corrected host reports partial match", + "A PVE host with leftover PBS files no longer offers PBS restore categories and no longer stops PBS services", }, }, } From 218ed93007fb2244b2b3996ce72efa8ed7202734 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:34:50 +0200 Subject: [PATCH 04/15] fix: the PBS validate brick stops concluding the host is PBS pbs_validate ran a bare Stat on the PBS configuration directory and logged "Detected %s, proceeding with PBS collection". That is a detection sentence, and the directory it stats is the one no package owns and no removal deletes, so on the host in issue #315 this was the third place in the codebase to conclude PBS from a leftover. It agreed with a type that was already wrong and let 25 more bricks run before the recipe hit the command that is not there. The type is settled before any recipe runs. Reaching this brick means PBS is installed, so a missing configuration directory is a genuine anomaly on a genuine PBS node rather than evidence about what the host is, and the error says that instead of "not a PBS system". No release note: the type is already correct by the time this runs, so the message only changes for an operator who is on a real PBS node with its configuration directory missing. --- internal/backup/collector_bricks_pbs.go | 14 ++++++++++++-- .../backup/collector_pbs_commands_coverage_test.go | 4 ++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/internal/backup/collector_bricks_pbs.go b/internal/backup/collector_bricks_pbs.go index 87a96d3f..31149091 100644 --- a/internal/backup/collector_bricks_pbs.go +++ b/internal/backup/collector_bricks_pbs.go @@ -18,14 +18,24 @@ func newPBSRecipe() recipe { c := state.collector c.logger.Debug("Validating PBS environment before collection") + // This is a sanity check on the path the recipe is about to read, not a + // second opinion on what the host is. It used to read as one: a bare Stat + // on a directory that no package owns and no removal deletes, logged as + // "Detected %s, proceeding with PBS collection". On the host in issue #315 + // that line was the third place in the codebase to conclude PBS from a + // leftover, and it agreed with a type that was already wrong. + // + // The type is settled before any recipe runs, so reaching here means PBS is + // installed. A missing config directory is then a real anomaly on a real + // PBS host, which is what the error says. pbsConfigPath := c.pbsConfigPath() if _, err := os.Stat(pbsConfigPath); err != nil { if errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("not a PBS system: %s not found", pbsConfigPath) + return fmt.Errorf("PBS is installed but %s is missing: the configuration directory a PBS node always has", pbsConfigPath) } return fmt.Errorf("failed to access PBS config path %s: %w", pbsConfigPath, err) } - c.logger.Debug("Detected %s, proceeding with PBS collection", pbsConfigPath) + c.logger.Debug("PBS configuration directory present at %s", pbsConfigPath) return nil }, }, diff --git a/internal/backup/collector_pbs_commands_coverage_test.go b/internal/backup/collector_pbs_commands_coverage_test.go index 20cb6624..f17e8d70 100644 --- a/internal/backup/collector_pbs_commands_coverage_test.go +++ b/internal/backup/collector_pbs_commands_coverage_test.go @@ -500,8 +500,8 @@ func TestCollectPBSConfigsReturnsErrorWhenNotPBSSystem(t *testing.T) { collector := NewCollector(newTestLogger(), cfg, t.TempDir(), types.ProxmoxBS, false) err := collector.CollectPBSConfigs(context.Background()) - if err == nil || !strings.Contains(err.Error(), "not a PBS system") { - t.Fatalf("expected not-a-PBS error, got %v", err) + if err == nil || !strings.Contains(err.Error(), "the configuration directory a PBS node always has") { + t.Fatalf("expected the missing-config-directory error, got %v", err) } } From 7790cf01a677c855f033b80acfa94ba7f6e7f426 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:42:52 +0200 Subject: [PATCH 05/15] fix: a failed role no longer discards the other role and the system payload Two independent losses, both visible on the host in issue #315. CollectAll returned as soon as the role-specific collection failed, and the common system collection sits after that switch. So a run that lost its PBS half also lost the network, storage-stack and hardware snapshot, which have nothing to do with which hypervisor the host is. The role error is now held until the common payload has been collected, and then returned unchanged. The dual recipe was PVE bricks and PBS bricks concatenated into one fail-fast list. On that host 34 PVE bricks and 25 PBS bricks completed, the sixtieth aborted on a command the host does not have, and the workspace holding all of it was deleted: the node ended the night with no backup because of a role it does not run. The halves now run as separate recipes over the shared state, and one failing leaves the other standing. Both failing is still an error, because then there is no role payload to keep and an archive of system files labelled dual would be worse than none. A kept half is not a quiet half. The role that did not finish is reported twice at warning and written into the manifest as incomplete_targets with its reason. Warning rather than error is the exit code: an error marks the run a failed backup, and this run produced an archive worth keeping, while a warning still promotes it off clean so a monitor sees the night was not normal. newDualRecipe has no caller left and goes, along with its entry in the recipe well-formedness test; the two recipes it concatenated are already covered there. --- internal/backup/collector.go | 37 ++++- internal/backup/collector_bricks.go | 8 - internal/backup/collector_bricks_test.go | 1 - internal/backup/collector_dual.go | 74 ++++++++- .../backup/collector_dual_partial_test.go | 151 ++++++++++++++++++ internal/backup/collector_manifest.go | 5 + internal/whatsnew/registry.go | 2 + 7 files changed, 259 insertions(+), 19 deletions(-) create mode 100644 internal/backup/collector_dual_partial_test.go diff --git a/internal/backup/collector.go b/internal/backup/collector.go index bd5519a7..2a4d0bf7 100644 --- a/internal/backup/collector.go +++ b/internal/backup/collector.go @@ -70,6 +70,19 @@ type Collector struct { // collectingCustomPaths is set while copying operator-supplied CUSTOM_BACKUP_PATHS, // during which the source walk prunes the staging workspace to avoid self-copy (#56). collectingCustomPaths bool + + // incomplete records a role whose collection aborted while the run carried on. + // It exists only for the dual case, where the two halves are independent: losing + // PBS is no reason to throw away the PVE payload that is already staged. Whatever + // lands here is reported and written into the manifest, so a partial archive is + // never mistaken for a whole one. + incomplete []incompleteTarget +} + +// incompleteTarget is one role that did not finish, and why. +type incompleteTarget struct { + Target string `json:"target"` + Reason string `json:"reason"` } var osSymlink = os.Symlink @@ -460,25 +473,33 @@ func (c *Collector) CollectAll(ctx context.Context) error { c.logger.Info("Starting backup collection for %s", c.proxType) c.logger.Debug("Collector dry-run=%v tempDir=%s", c.dryRun, c.tempDir) + // roleErr is held rather than returned on the spot. The common system payload + // below is independent of which hypervisor this is, and returning here skipped it: + // a run that lost its PBS half also lost the network, storage-stack and hardware + // snapshot it had nothing to do with (issue #315). + var roleErr error switch c.proxType { case types.ProxmoxVE: c.logger.Debug("Invoking PVE-specific collectors (configs, jobs, schedules, storage metadata)") if err := c.CollectPVEConfigs(ctx); err != nil { - return fmt.Errorf("PVE collection failed: %w", err) + roleErr = fmt.Errorf("PVE collection failed: %w", err) + } else { + c.logger.Debug("PVE-specific collection completed") } - c.logger.Debug("PVE-specific collection completed") case types.ProxmoxBS: c.logger.Debug("Invoking PBS-specific collectors (datastores, users, namespaces, pxar metadata)") if err := c.CollectPBSConfigs(ctx); err != nil { - return fmt.Errorf("PBS collection failed: %w", err) + roleErr = fmt.Errorf("PBS collection failed: %w", err) + } else { + c.logger.Debug("PBS-specific collection completed") } - c.logger.Debug("PBS-specific collection completed") case types.ProxmoxDual: c.logger.Debug("Invoking dual-role collectors (PVE + PBS recipes, shared system/common once)") if err := c.CollectDualConfigs(ctx); err != nil { - return fmt.Errorf("dual collection failed: %w", err) + roleErr = fmt.Errorf("dual collection failed: %w", err) + } else { + c.logger.Debug("Dual-role collection completed") } - c.logger.Debug("Dual-role collection completed") case types.ProxmoxUnknown: c.logger.Warning("Unknown Proxmox type, collecting generic system info only") c.logger.Debug("Skipping hypervisor-specific collection because type is unknown") @@ -494,6 +515,10 @@ func (c *Collector) CollectAll(ctx context.Context) error { } c.logger.Debug("Baseline system information collected successfully") + if roleErr != nil { + return roleErr + } + stats := c.GetStats() c.logger.Debug("Collection completed: %d files, %d failed, %d dirs created", stats.FilesProcessed, stats.FilesFailed, stats.DirsCreated) diff --git a/internal/backup/collector_bricks.go b/internal/backup/collector_bricks.go index 0bc0db5e..b6287bb8 100644 --- a/internal/backup/collector_bricks.go +++ b/internal/backup/collector_bricks.go @@ -417,11 +417,3 @@ func (s *collectionState) ensurePVERuntimeInfo() *pveRuntimeInfo { return s.pve.runtimeInfo } -func newDualRecipe() recipe { - bricks := append([]collectionBrick{}, newPVERecipe().Bricks...) - bricks = append(bricks, newPBSRecipe().Bricks...) - return recipe{ - Name: "dual", - Bricks: bricks, - } -} diff --git a/internal/backup/collector_bricks_test.go b/internal/backup/collector_bricks_test.go index eea54edc..9088fa44 100644 --- a/internal/backup/collector_bricks_test.go +++ b/internal/backup/collector_bricks_test.go @@ -210,7 +210,6 @@ func TestRealRecipesHaveCompleteUniqueBricks(t *testing.T) { newPBSPXARRecipe(), newPBSUserConfigRecipe(), newSystemRecipe(), - newDualRecipe(), } for _, r := range recipes { diff --git a/internal/backup/collector_dual.go b/internal/backup/collector_dual.go index 3274dc70..1c18856d 100644 --- a/internal/backup/collector_dual.go +++ b/internal/backup/collector_dual.go @@ -1,15 +1,81 @@ package backup -import "context" +import ( + "context" + "fmt" +) // CollectDualConfigs collects both PVE and PBS configurations on a coinstalled host. +// +// The two halves run as separate recipes against one shared state. They used to be one +// concatenated recipe, and runRecipe is fail-fast, so an abort anywhere in the PBS half +// ended the whole thing: on the host in issue #315 that meant 34 PVE bricks and 25 PBS +// bricks completed, the sixtieth aborted, and the workspace holding all of it was +// deleted. The host got no backup at all because of a role it does not even have. +// +// A half that fails is recorded and reported, and the other half is kept. Both failing +// is still an error, because then there is no role payload left to keep: the caller +// stops the run rather than shipping an archive of system files labelled dual. func (c *Collector) CollectDualConfigs(ctx context.Context) error { c.logger.Info("Collecting dual-role configurations") state := newCollectionState(c) - if err := runRecipe(ctx, newDualRecipe(), state); err != nil { - return err + + pveErr := runRecipe(ctx, newPVERecipe(), state) + if pveErr != nil && isContextCancellationError(ctx, pveErr) { + return pveErr + } + + pbsErr := runRecipe(ctx, newPBSRecipe(), state) + if pbsErr != nil && isContextCancellationError(ctx, pbsErr) { + return pbsErr + } + + switch { + case pveErr != nil && pbsErr != nil: + return fmt.Errorf("both halves failed: PVE: %w; PBS: %v", pveErr, pbsErr) + case pveErr != nil: + c.noteIncompleteTarget("pve", pveErr) + case pbsErr != nil: + c.noteIncompleteTarget("pbs", pbsErr) } - c.logger.Info("Dual-role configuration collection completed") + if pveErr == nil && pbsErr == nil { + c.logger.Info("Dual-role configuration collection completed") + } return nil } + +// noteIncompleteTarget records a role that did not finish. +// +// It reports at warning, not error. The distinction is the exit code: an error makes +// the run a failed backup (ExitBackupError), and this run produced an archive that is +// worth keeping. A warning promotes a clean run to the generic exit code, so a monitor +// still sees that the night was not normal without being told the backup failed when +// it did not. The manifest carries the same record, so the gap travels with the +// archive rather than living only in a log that may not be kept. +func (c *Collector) noteIncompleteTarget(target string, cause error) { + if cause == nil { + return + } + reason := cause.Error() + + c.statsMu.Lock() + c.incomplete = append(c.incomplete, incompleteTarget{Target: target, Reason: reason}) + c.statsMu.Unlock() + + c.logger.Warning("Collection: the %s half of this dual host did not finish - %s", target, reason) + c.logger.Warning("Collection: this backup carries the other role and the system payload, and is marked incomplete for %s", target) +} + +// IncompleteTargets returns the roles whose collection did not finish, for a caller +// that reports run status. Empty on a whole backup. +func (c *Collector) IncompleteTargets() []string { + c.statsMu.Lock() + defer c.statsMu.Unlock() + + targets := make([]string, 0, len(c.incomplete)) + for _, entry := range c.incomplete { + targets = append(targets, entry.Target) + } + return targets +} diff --git a/internal/backup/collector_dual_partial_test.go b/internal/backup/collector_dual_partial_test.go new file mode 100644 index 00000000..2d3e2666 --- /dev/null +++ b/internal/backup/collector_dual_partial_test.go @@ -0,0 +1,151 @@ +package backup + +import ( + "context" + "errors" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tis24dev/proxsave/internal/logging" + "github.com/tis24dev/proxsave/internal/types" +) + +// dualCollector builds a dual collector over a fake system root. Whichever of the two +// configuration directories the caller asks for is created; the other is absent, which +// is how each half is made to fail at its validate brick. +func dualCollector(t *testing.T, withPVE, withPBS bool) *Collector { + t.Helper() + logger := logging.New(types.LogLevelError, false) + logger.SetOutput(io.Discard) + + systemRoot := t.TempDir() + if withPVE { + if err := os.MkdirAll(filepath.Join(systemRoot, "etc", "pve"), 0o755); err != nil { + t.Fatalf("mkdir /etc/pve: %v", err) + } + } + if withPBS { + if err := os.MkdirAll(filepath.Join(systemRoot, "etc", "proxmox-backup"), 0o755); err != nil { + t.Fatalf("mkdir /etc/proxmox-backup: %v", err) + } + } + + cfg := GetDefaultCollectorConfig() + cfg.SystemRootPrefix = systemRoot + + deps := defaultCollectorDeps() + deps.LookPath = func(name string) (string, error) { + switch name { + case "cat", "uname", "pveversion", "proxmox-backup-manager": + return "/bin/true", nil + default: + return "", errors.New("missing") + } + } + deps.RunCommand = func(context.Context, string, ...string) ([]byte, error) { + return []byte("[]"), nil + } + + return NewCollectorWithDeps(logger, cfg, t.TempDir(), types.ProxmoxDual, true, deps) +} + +// TestDualKeepsThePVEHalfWhenPBSFails is the loss issue #315 caused. The two halves +// used to be one fail-fast recipe, so an abort in the PBS half threw away the PVE +// payload that had already been collected and the host ended the night with nothing. +func TestDualKeepsThePVEHalfWhenPBSFails(t *testing.T) { + collector := dualCollector(t, true, false) + + if err := collector.CollectAll(context.Background()); err != nil { + t.Fatalf("CollectAll returned %v, want the run to carry on with the half it has", err) + } + + incomplete := collector.IncompleteTargets() + if len(incomplete) != 1 || incomplete[0] != "pbs" { + t.Fatalf("IncompleteTargets() = %v, want exactly [pbs]", incomplete) + } + if len(collector.pveManifest) == 0 { + t.Fatal("the PVE manifest is empty: the half that succeeded was not kept") + } +} + +// TestDualKeepsThePBSHalfWhenPVEFails is the same contract from the other side, so the +// behaviour is a property of the recipe pair rather than of the order they run in. +func TestDualKeepsThePBSHalfWhenPVEFails(t *testing.T) { + collector := dualCollector(t, false, true) + + if err := collector.CollectAll(context.Background()); err != nil { + t.Fatalf("CollectAll returned %v, want the run to carry on with the half it has", err) + } + + incomplete := collector.IncompleteTargets() + if len(incomplete) != 1 || incomplete[0] != "pve" { + t.Fatalf("IncompleteTargets() = %v, want exactly [pve]", incomplete) + } +} + +// TestDualFailsWhenBothHalvesFail: with no role payload left there is nothing to keep, +// and shipping an archive of system files labelled dual would be worse than failing. +func TestDualFailsWhenBothHalvesFail(t *testing.T) { + collector := dualCollector(t, false, false) + + err := collector.CollectAll(context.Background()) + if err == nil { + t.Fatal("CollectAll returned nil, want an error when neither half produced anything") + } + if !strings.Contains(err.Error(), "both halves failed") { + t.Fatalf("CollectAll error = %v, want it to say both halves failed", err) + } +} + +// TestIncompleteTargetTravelsInTheManifest: a log can be rotated away or never read, +// and the archive then looks whole. The manifest is what goes with it. +func TestIncompleteTargetTravelsInTheManifest(t *testing.T) { + collector := dualCollector(t, true, false) + if err := collector.CollectAll(context.Background()); err != nil { + t.Fatalf("CollectAll: %v", err) + } + + if len(collector.incomplete) != 1 { + t.Fatalf("collector.incomplete = %v, want one entry", collector.incomplete) + } + entry := collector.incomplete[0] + if entry.Target != "pbs" { + t.Fatalf("incomplete target = %q, want pbs", entry.Target) + } + if strings.TrimSpace(entry.Reason) == "" { + t.Fatal("the recorded reason is empty: the manifest would say a half is missing without saying why") + } +} + +// TestSystemPayloadSurvivesAFailedRole: the common payload has nothing to do with +// which hypervisor this is, and a bare return used to skip it along with the rest. +func TestSystemPayloadSurvivesAFailedRole(t *testing.T) { + logger := logging.New(types.LogLevelError, false) + logger.SetOutput(io.Discard) + + cfg := GetDefaultCollectorConfig() + cfg.SystemRootPrefix = t.TempDir() + cfg.PVEConfigPath = filepath.Join(t.TempDir(), "missing") + + deps := defaultCollectorDeps() + deps.LookPath = func(name string) (string, error) { + switch name { + case "cat", "uname": + return "/bin/true", nil + default: + return "", errors.New("missing") + } + } + + collector := NewCollectorWithDeps(logger, cfg, t.TempDir(), types.ProxmoxVE, true, deps) + err := collector.CollectAll(context.Background()) + if err == nil || !strings.Contains(err.Error(), "PVE collection failed:") { + t.Fatalf("CollectAll error = %v, want the single-role failure still reported", err) + } + if len(collector.systemManifest) == 0 { + t.Fatal("the system manifest is empty: the common payload was skipped because the role failed") + } +} diff --git a/internal/backup/collector_manifest.go b/internal/backup/collector_manifest.go index 88c87c0e..f25b21d8 100644 --- a/internal/backup/collector_manifest.go +++ b/internal/backup/collector_manifest.go @@ -35,6 +35,10 @@ type BackupManifest struct { Hostname string `json:"hostname"` ProxmoxType string `json:"proxmox_type"` ProxmoxTargets []string `json:"proxmox_targets,omitempty"` + // Incomplete names any role whose collection aborted while the run carried on. + // Absent on a whole backup. It is what stops a partial archive from reading like + // a complete one once the run log is gone. + Incomplete []incompleteTarget `json:"incomplete_targets,omitempty"` PBSConfigs map[string]ManifestEntry `json:"pbs_configs,omitempty"` PVEConfigs map[string]ManifestEntry `json:"pve_configs,omitempty"` SystemFiles map[string]ManifestEntry `json:"system_files,omitempty"` @@ -58,6 +62,7 @@ func (c *Collector) WriteManifest(hostname string) error { Hostname: hostname, ProxmoxType: string(c.proxType), ProxmoxTargets: append([]string(nil), c.proxType.Targets()...), + Incomplete: append([]incompleteTarget(nil), c.incomplete...), PBSConfigs: c.pbsManifest, PVEConfigs: c.pveManifest, SystemFiles: c.systemManifest, diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index 4193dfd2..04995809 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -207,12 +207,14 @@ var notes = []Note{ "A PVE host with leftover proxmox-backup files is no longer taken for a PBS server, and its backup runs again", "A run says when a product left files behind without being installed, and names the file it found", "Restore reads the host type from the same check the backup uses, instead of keeping a weaker one of its own", + "On a PVE plus PBS host, one role failing no longer discards the other role and the shared system payload", }, Actions: []string{ "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", "The leftover directories are still reported: remove them, or install PBS, only if you want the notice to stop", "An archive taken before this fix is labelled dual, so restoring it on the corrected host reports partial match", "A PVE host with leftover PBS files no longer offers PBS restore categories and no longer stops PBS services", + "A dual backup that lost one role exits with the warning code and names the missing role in its manifest", }, }, } From abe18266a5e3c2fc2e3e18880f24f3f563ace801 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:44:03 +0200 Subject: [PATCH 06/15] fix: the marker table reports both dpkg probes, present or not The table is an inventory: every marker in it answers YES or NO. These two answered only when the package was installed, which is the one case that needs no explaining. On the host in issue #315 the table listed nine PBS markers, all of them NO, and silently omitted the tenth. The omitted one was the decisive evidence: that proxmox-backup-server is not installed at all. An absent line reads as a check that never ran rather than as a negative answer, so the table said least exactly where it mattered most, and the installed version goes on the line now too. --- internal/environment/detect.go | 16 +++++++++++----- internal/environment/detect_residue_test.go | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/internal/environment/detect.go b/internal/environment/detect.go index 1ac5f6f2..4c886818 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -965,11 +965,17 @@ func markerLines() []string { add("%s exists: %s", resolveUnderPrefix(pveShareDir), boolToYes(dirExists(pveShareDir))) add("%s exists: %s", resolveUnderPrefix(pbsShareDir), boolToYes(dirExists(pbsShareDir))) add("%s exists: %s", resolveUnderPrefix(dpkgStatusFile), boolToYes(fileExists(dpkgStatusFile))) - if _, ok := dpkgPackageInstalled("pve-manager"); ok { - add("dpkg pve-manager: installed") - } - if _, ok := dpkgPackageInstalled("proxmox-backup-server"); ok { - add("dpkg proxmox-backup-server: installed") + // Both lines are printed whichever way they come out. They used to print only when + // the package was installed, which is the one case that needs no explaining: on the + // host in issue #315 the table listed nine PBS markers and silently omitted the one + // that said proxmox-backup-server is NOT installed, so the decisive evidence read as + // a check that had never run. Every other marker here reports YES or NO. + for _, pkg := range []string{"pve-manager", "proxmox-backup-server"} { + if version, ok := dpkgPackageInstalled(pkg); ok { + add("dpkg %s: installed (%s)", pkg, version) + } else { + add("dpkg %s: not installed", pkg) + } } add("") diff --git a/internal/environment/detect_residue_test.go b/internal/environment/detect_residue_test.go index 1240f2b0..fc28b7f7 100644 --- a/internal/environment/detect_residue_test.go +++ b/internal/environment/detect_residue_test.go @@ -241,3 +241,23 @@ func TestEmptyPBSVersionFileIsResidue(t *testing.T) { func dpkgStanza(pkg, version string) string { return "Package: " + pkg + "\nStatus: install ok installed\nVersion: " + version + "\n\n" } + +// TestMarkerTableReportsBothDpkgProbesEitherWay: the table is an inventory, and every +// other marker in it reports YES or NO. These two used to print only when the package +// was installed, so on the host in issue #315 the line that would have said +// proxmox-backup-server is absent was simply missing, and an absent line reads as a +// check that never ran rather than as a negative answer. +func TestMarkerTableReportsBothDpkgProbesEitherWay(t *testing.T) { + root := t.TempDir() + writeFile(t, filepath.Join(root, "var/lib/dpkg/status"), dpkgStanza("pve-manager", "9.2.18")) + + joined := strings.Join(MarkerSnapshot(DetectOptions{RootPrefix: root}), "\n") + for _, want := range []string{ + "dpkg pve-manager: installed (9.2.18)", + "dpkg proxmox-backup-server: not installed", + } { + if !strings.Contains(joined, want) { + t.Fatalf("snapshot missing %q:\n%s", want, joined) + } + } +} From 9de04d673791a882b75ad22f21de4d707014a4cb Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:45:26 +0200 Subject: [PATCH 07/15] chore: drop the three detection helpers only tests called detectPVEViaSources, detectPBSViaSources and detectViaDirectories stopped being part of detection when the ladder inlined its rungs, and nothing in the package has called them since. Six tests kept calling them, so the package looked covered where production code no longer ran: two of those tests only asserted that the function does not panic. The cases worth keeping are pointed at the helpers detection actually uses, firstMatchingSource and firstExistingDir, so the same behaviour stays covered on the code path that runs. No release note: nothing an operator can observe. --- internal/environment/detect.go | 9 -------- .../environment/detect_additional_test.go | 22 ++----------------- .../environment/detect_deterministic_test.go | 16 +++++++------- 3 files changed, 10 insertions(+), 37 deletions(-) diff --git a/internal/environment/detect.go b/internal/environment/detect.go index 4c886818..dc468d00 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -741,17 +741,8 @@ func detectPBSViaVersionFile() (string, markerOutcome) { return "", markerResidual } -func detectPVEViaSources() bool { - return firstMatchingSource(pveSourceFiles, pveSourceTokens) != "" -} -func detectPBSViaSources() bool { - return firstMatchingSource(pbsSourceFiles, pbsSourceTokens) != "" -} -func detectViaDirectories(paths []string) bool { - return firstExistingDir(paths) != "" -} func extendPath() { currentPath := os.Getenv("PATH") diff --git a/internal/environment/detect_additional_test.go b/internal/environment/detect_additional_test.go index 93c3cc8b..e7b42660 100644 --- a/internal/environment/detect_additional_test.go +++ b/internal/environment/detect_additional_test.go @@ -225,32 +225,14 @@ func TestDetectViaDirectories(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := detectViaDirectories(tt.paths) + result := firstExistingDir(tt.paths) != "" if result != tt.expected { - t.Errorf("detectViaDirectories() = %v, want %v", result, tt.expected) + t.Errorf("firstExistingDir() found = %v, want %v", result, tt.expected) } }) } } -// TestDetectPVEViaSources tests PVE detection via apt sources -func TestDetectPVEViaSources(t *testing.T) { - // Since this function checks actual system paths, we test the logic - // On most systems this will return false - result := detectPVEViaSources() - // Just verify it doesn't panic - _ = result -} - -// TestDetectPBSViaSources tests PBS detection via apt sources -func TestDetectPBSViaSources(t *testing.T) { - // Since this function checks actual system paths, we test the logic - // On most systems this will return false - result := detectPBSViaSources() - // Just verify it doesn't panic - _ = result -} - // TestExtendPath tests PATH environment variable extension func TestExtendPath(t *testing.T) { t.Setenv("PATH", "/usr/local/bin") diff --git a/internal/environment/detect_deterministic_test.go b/internal/environment/detect_deterministic_test.go index 779b4d14..c6e2f2eb 100644 --- a/internal/environment/detect_deterministic_test.go +++ b/internal/environment/detect_deterministic_test.go @@ -266,11 +266,11 @@ func TestDetectViaSources_Branches(t *testing.T) { setValue(t, &pveSourceFiles, []string{pveSource}) setValue(t, &pbsSourceFiles, []string{pbsSource}) - if !detectPVEViaSources() { - t.Fatal("detectPVEViaSources() should be true") + if firstMatchingSource(pveSourceFiles, pveSourceTokens) == "" { + t.Fatal("the PVE source file should match its token") } - if !detectPBSViaSources() { - t.Fatal("detectPBSViaSources() should be true") + if firstMatchingSource(pbsSourceFiles, pbsSourceTokens) == "" { + t.Fatal("the PBS source file should match its token") } emptySource := filepath.Join(tmpDir, "empty.list") @@ -280,11 +280,11 @@ func TestDetectViaSources_Branches(t *testing.T) { setValue(t, &pveSourceFiles, []string{emptySource}) setValue(t, &pbsSourceFiles, []string{emptySource}) - if detectPVEViaSources() { - t.Fatal("detectPVEViaSources() should be false") + if path := firstMatchingSource(pveSourceFiles, pveSourceTokens); path != "" { + t.Fatalf("a source file with no product token matched PVE: %s", path) } - if detectPBSViaSources() { - t.Fatal("detectPBSViaSources() should be false") + if path := firstMatchingSource(pbsSourceFiles, pbsSourceTokens); path != "" { + t.Fatalf("a source file with no product token matched PBS: %s", path) } } From 708528cc3fdcc8b277b1b7fba00b9dea49ecb7b4 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:48:43 +0200 Subject: [PATCH 08/15] fix: a version command that answers late no longer costs the run its version The command rung returned "installed, version unknown" when the probe failed, and that stopped the ladder one step above dpkg, which holds the real version. So a run reported a host with no version at all while the number sat in the next rung down. It is not a rare path. pveversion takes 4.4 to 5.2 seconds on pve-test and commandTimeout is 5, so it times out on nothing more unusual than a busy node. Before this, the probe on that host printed PVEVersion "" and a combined version of the PBS half alone; after it, pve=9.1.9,pbs=4.2.0. The probes now report three outcomes instead of two. markerInstalledNoVersion says the binary is on PATH, so the product IS installed, and this run did not produce a version: the ladder keeps walking for one, and falls back to "unknown" only if every later marker misses too. It is deliberately not markerResidual, which says the opposite about whether the product is there. Found while verifying the issue #315 fix on a real host, not part of that fix. It is its own commit so it can be judged, or reverted, on its own. --- internal/environment/detect.go | 75 ++++++++++++++----- .../environment/detect_deterministic_test.go | 48 ++++++------ internal/environment/detect_residue_test.go | 63 ++++++++++++++++ internal/whatsnew/registry.go | 1 + 4 files changed, 145 insertions(+), 42 deletions(-) diff --git a/internal/environment/detect.go b/internal/environment/detect.go index dc468d00..ccf7896e 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -452,13 +452,21 @@ func detectPVE(trace *detectionTrace) (string, bool, string) { } } + installedWithoutVersion := false if hostRooted() { trace.skip(productPVE, "command", "pveversion", "a command run here answers for the appliance, not for the mounted host") - } else if version, ok := detectPVEViaCommand(); ok { - trace.hit(productPVE, "command", "pveversion", version) - return version, true, "" } else { - trace.miss(productPVE, "command", "pveversion") + switch version, outcome := detectPVEViaCommand(); outcome { + case markerInstalled: + trace.hit(productPVE, "command", "pveversion", version) + return version, true, "" + case markerInstalledNoVersion: + installedWithoutVersion = true + trace.add(DetectionStep{Product: productPVE, Marker: "command", Target: "pveversion", Hit: true, + Note: "the command is installed but gave no version; looking for one further down"}) + default: + trace.miss(productPVE, "command", "pveversion") + } } versionFiles := targetList(pveVersionFile, pveLegacyFile) @@ -511,6 +519,9 @@ func detectPVE(trace *detectionTrace) (string, bool, string) { trace.miss(productPVE, "directory", targetList(pveDirCandidates...)) } + if installedWithoutVersion { + return "unknown", true, "" + } return "", false, residue } @@ -536,13 +547,21 @@ func detectPBS(trace *detectionTrace) (string, bool, string) { } } + installedWithoutVersion := false if hostRooted() { trace.skip(productPBS, "command", "proxmox-backup-manager", "a command run here answers for the appliance, not for the mounted host") - } else if version, ok := detectPBSViaCommand(); ok { - trace.hit(productPBS, "command", "proxmox-backup-manager", version) - return version, true, "" } else { - trace.miss(productPBS, "command", "proxmox-backup-manager") + switch version, outcome := detectPBSViaCommand(); outcome { + case markerInstalled: + trace.hit(productPBS, "command", "proxmox-backup-manager", version) + return version, true, "" + case markerInstalledNoVersion: + installedWithoutVersion = true + trace.add(DetectionStep{Product: productPBS, Marker: "command", Target: "proxmox-backup-manager", Hit: true, + Note: "the command is installed but gave no version; looking for one further down"}) + default: + trace.miss(productPBS, "command", "proxmox-backup-manager") + } } switch version, outcome := detectPBSViaVersionFile(); outcome { @@ -588,6 +607,9 @@ func detectPBS(trace *detectionTrace) (string, bool, string) { trace.miss(productPBS, "directory", targetList(pbsDirCandidates...)) } + if installedWithoutVersion { + return "unknown", true, "" + } return "", false, residue } @@ -662,40 +684,52 @@ func dpkgStanzaField(stanza, key string) string { return "" } -func detectPVEViaCommand() (string, bool) { +// detectPVEViaCommand runs pveversion. The binary being on PATH already proves PVE is +// installed, so the outcome is never "absent" once lookPath succeeds; what the run adds +// is the version, and it can fail to add it. +// +// That failure is not rare. pveversion takes 4.4 to 5.2 seconds on the lab host and +// commandTimeout is 5, so it times out intermittently on nothing more unusual than a +// busy node. The caller uses markerInstalled to stop the ladder and markerResidual to +// carry on, so a run that got the binary but not the version keeps walking and lets +// dpkg supply it, instead of settling for "unknown" with the real version one rung +// further down. +func detectPVEViaCommand() (string, markerOutcome) { cmdPath, err := lookPathFunc("pveversion") if err != nil { - return "", false + return "", markerAbsent } output, err := runCommandFunc(cmdPath) if err != nil { - return "unknown", true + return "", markerInstalledNoVersion } version := extractPVEVersion(output) if version == "" { - return "unknown", true + return "", markerInstalledNoVersion } - return version, true + return version, markerInstalled } -func detectPBSViaCommand() (string, bool) { +// detectPBSViaCommand is the PBS half, with the same three outcomes as +// detectPVEViaCommand and for the same reason. +func detectPBSViaCommand() (string, markerOutcome) { cmdPath, err := lookPathFunc("proxmox-backup-manager") if err != nil { - return "", false + return "", markerAbsent } output, err := runCommandFunc(cmdPath, "version") if err != nil { - return "unknown", true + return "", markerInstalledNoVersion } version := extractPBSVersion(output) if version == "" { - return "unknown", true + return "", markerInstalledNoVersion } - return version, true + return version, markerInstalled } // markerOutcome is what a version-file probe found. The middle state is the point: @@ -706,7 +740,12 @@ type markerOutcome int const ( markerAbsent markerOutcome = iota + // markerResidual: something is there that does not prove an install. markerResidual + // markerInstalledNoVersion: the product IS installed and this probe could not say + // which version. Distinct from markerResidual, which says the opposite about the + // product, and distinct from markerInstalled, which ends the ladder. + markerInstalledNoVersion markerInstalled ) diff --git a/internal/environment/detect_deterministic_test.go b/internal/environment/detect_deterministic_test.go index c6e2f2eb..e158c329 100644 --- a/internal/environment/detect_deterministic_test.go +++ b/internal/environment/detect_deterministic_test.go @@ -61,9 +61,9 @@ func TestDetectPVEViaCommand_Branches(t *testing.T) { t.Run("not found", func(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "", errors.New("not found") }) - version, ok := detectPVEViaCommand() - if ok || version != "" { - t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v)", version, ok, "", false) + version, outcome := detectPVEViaCommand() + if outcome != markerAbsent || version != "" { + t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v)", version, outcome, "", markerAbsent) } }) @@ -71,9 +71,9 @@ func TestDetectPVEViaCommand_Branches(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "/fake/pveversion", nil }) setValue(t, &runCommandFunc, func(string, ...string) (string, error) { return "", errors.New("boom") }) - version, ok := detectPVEViaCommand() - if !ok || version != "unknown" { - t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, outcome := detectPVEViaCommand() + if outcome != markerInstalledNoVersion || version != "" { + t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v): the binary proves the install, the run did not give a version", version, outcome, "", markerInstalledNoVersion) } }) @@ -83,9 +83,9 @@ func TestDetectPVEViaCommand_Branches(t *testing.T) { return "no version here", nil }) - version, ok := detectPVEViaCommand() - if !ok || version != "unknown" { - t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, outcome := detectPVEViaCommand() + if outcome != markerInstalledNoVersion || version != "" { + t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v): the binary proves the install, the run did not give a version", version, outcome, "", markerInstalledNoVersion) } }) @@ -95,9 +95,9 @@ func TestDetectPVEViaCommand_Branches(t *testing.T) { return "pve-manager/7.4-3/d4a3b4a1 (running kernel: 5.15.35-1-pve)", nil }) - version, ok := detectPVEViaCommand() - if !ok || version != "7.4-3" { - t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v)", version, ok, "7.4-3", true) + version, outcome := detectPVEViaCommand() + if outcome != markerInstalled || version != "7.4-3" { + t.Fatalf("detectPVEViaCommand() = (%q, %v), want (%q, %v)", version, outcome, "7.4-3", markerInstalled) } }) } @@ -106,9 +106,9 @@ func TestDetectPBSViaCommand_Branches(t *testing.T) { t.Run("not found", func(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "", errors.New("not found") }) - version, ok := detectPBSViaCommand() - if ok || version != "" { - t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v)", version, ok, "", false) + version, outcome := detectPBSViaCommand() + if outcome != markerAbsent || version != "" { + t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v)", version, outcome, "", markerAbsent) } }) @@ -116,9 +116,9 @@ func TestDetectPBSViaCommand_Branches(t *testing.T) { setValue(t, &lookPathFunc, func(string) (string, error) { return "/fake/proxmox-backup-manager", nil }) setValue(t, &runCommandFunc, func(string, ...string) (string, error) { return "", errors.New("boom") }) - version, ok := detectPBSViaCommand() - if !ok || version != "unknown" { - t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, outcome := detectPBSViaCommand() + if outcome != markerInstalledNoVersion || version != "" { + t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v): the binary proves the install, the run did not give a version", version, outcome, "", markerInstalledNoVersion) } }) @@ -128,9 +128,9 @@ func TestDetectPBSViaCommand_Branches(t *testing.T) { return "proxmox-backup-manager 2.4.1\nno version here", nil }) - version, ok := detectPBSViaCommand() - if !ok || version != "unknown" { - t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v)", version, ok, "unknown", true) + version, outcome := detectPBSViaCommand() + if outcome != markerInstalledNoVersion || version != "" { + t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v): the binary proves the install, the run did not give a version", version, outcome, "", markerInstalledNoVersion) } }) @@ -140,9 +140,9 @@ func TestDetectPBSViaCommand_Branches(t *testing.T) { return "proxmox-backup-manager 2.4.1\nversion: 2.4.1", nil }) - version, ok := detectPBSViaCommand() - if !ok || version != "2.4.1" { - t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v)", version, ok, "2.4.1", true) + version, outcome := detectPBSViaCommand() + if outcome != markerInstalled || version != "2.4.1" { + t.Fatalf("detectPBSViaCommand() = (%q, %v), want (%q, %v)", version, outcome, "2.4.1", markerInstalled) } }) } diff --git a/internal/environment/detect_residue_test.go b/internal/environment/detect_residue_test.go index fc28b7f7..60a047a9 100644 --- a/internal/environment/detect_residue_test.go +++ b/internal/environment/detect_residue_test.go @@ -261,3 +261,66 @@ func TestMarkerTableReportsBothDpkgProbesEitherWay(t *testing.T) { } } } + +// TestATimedOutCommandStillGetsItsVersionFromDpkg is measured, not hypothetical: +// pveversion takes 4.4 to 5.2 seconds on the lab host and commandTimeout is 5, so it +// times out on nothing more unusual than a busy node. The rung used to answer +// "installed, version unknown" and stop the ladder one step above dpkg, which holds +// the real version, leaving the run to report a host with no version at all. +func TestATimedOutCommandStillGetsItsVersionFromDpkg(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + + dpkgStatus := filepath.Join(tmp, "dpkg-status") + writeFile(t, dpkgStatus, dpkgStanza("pve-manager", "9.2.18")) + setValue(t, &dpkgStatusFile, dpkgStatus) + + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + if cmd == "pveversion" { + return "/usr/bin/pveversion", nil + } + return "", errors.New("not found") + }) + setValue(t, &runCommandFunc, func(string, ...string) (string, error) { + return "", errors.New("command pveversion timed out") + }) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) + } + if info.PVEVersion != "9.2.18" { + t.Fatalf("PVEVersion = %q, want 9.2.18 recovered from dpkg", info.PVEVersion) + } +} + +// TestAVersionlessCommandStillProvesTheInstall: if every version-bearing marker is +// gone too, the host is still PVE. The binary on PATH is the proof; only the version +// is missing. +func TestAVersionlessCommandStillProvesTheInstall(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + if cmd == "pveversion" { + return "/usr/bin/pveversion", nil + } + return "", errors.New("not found") + }) + setValue(t, &runCommandFunc, func(string, ...string) (string, error) { + return "no version in this output", nil + }) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) + } +} diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index 04995809..036dab13 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -208,6 +208,7 @@ var notes = []Note{ "A run says when a product left files behind without being installed, and names the file it found", "Restore reads the host type from the same check the backup uses, instead of keeping a weaker one of its own", "On a PVE plus PBS host, one role failing no longer discards the other role and the shared system payload", + "A slow pveversion no longer costs the run its version number: the version is read from the package instead", }, Actions: []string{ "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", From f6efdfc223446882f9dfd915bb13a5ce304351be Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 19:53:43 +0200 Subject: [PATCH 09/15] test: cover the residue warning, including the silence it has to keep warnDetectionResidue landed with the detection fix and had no test. The case worth pinning is the quiet one: detection returns at the first marker that proves an install and never reaches the residue rungs, so a healthy host of either kind records nothing and must print nothing. A line that fired on every run would be ignored by the time it mattered, which is the run where the type is not what the operator expected. Also pinned: one line per product that left something behind, and nil arguments, since this runs on the bootstrap path before the main logger exists. --- cmd/proxsave/main_runtime_residue_test.go | 71 +++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 cmd/proxsave/main_runtime_residue_test.go diff --git a/cmd/proxsave/main_runtime_residue_test.go b/cmd/proxsave/main_runtime_residue_test.go new file mode 100644 index 00000000..c37e2b54 --- /dev/null +++ b/cmd/proxsave/main_runtime_residue_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "testing" + + "github.com/tis24dev/proxsave/internal/environment" + "github.com/tis24dev/proxsave/internal/logging" + "github.com/tis24dev/proxsave/internal/types" +) + +// TestResidueWarningStaysQuietOnAHealthyHost is the one that keeps this line useful. +// Detection returns at the first marker that proves an install and never reaches the +// residue rungs, so a host that has the product records no residue and must produce no +// warning. A line that fired on every run would be ignored by the time it mattered. +func TestResidueWarningStaysQuietOnAHealthyHost(t *testing.T) { + for _, info := range []*environment.EnvironmentInfo{ + {Type: types.ProxmoxVE}, + {Type: types.ProxmoxBS}, + {Type: types.ProxmoxDual}, + {Type: types.ProxmoxVE, PVEResidual: " "}, + } { + bootstrap := logging.NewBootstrapLogger() + before := bootstrap.EntryCount() + warnDetectionResidue(bootstrap, info) + if got := bootstrap.EntryCount() - before; got != 0 { + t.Fatalf("type %s with residual %q logged %d entries, want silence", info.Type, info.PVEResidual, got) + } + } +} + +// TestResidueWarningFiresOncePerProduct: the host in issue #315 has PBS residue and no +// PBS, and the operator looking at a type they did not expect never sees the trace, +// which is debug only. +func TestResidueWarningFiresOncePerProduct(t *testing.T) { + for _, tc := range []struct { + name string + info *environment.EnvironmentInfo + want int + }{ + { + name: "pbs residue on a pve host", + info: &environment.EnvironmentInfo{Type: types.ProxmoxVE, PBSResidual: "directory (/etc/proxmox-backup)"}, + want: 1, + }, + { + name: "both products left something behind", + info: &environment.EnvironmentInfo{ + Type: types.ProxmoxUnknown, + PVEResidual: "directory (/etc/pve)", + PBSResidual: "directory (/etc/proxmox-backup)", + }, + want: 2, + }, + } { + t.Run(tc.name, func(t *testing.T) { + bootstrap := logging.NewBootstrapLogger() + before := bootstrap.EntryCount() + warnDetectionResidue(bootstrap, tc.info) + if got := bootstrap.EntryCount() - before; got != tc.want { + t.Fatalf("logged %d entries, want %d", got, tc.want) + } + }) + } +} + +// TestResidueWarningSurvivesNilArguments: it runs on the bootstrap path, before the +// main logger exists, and must not be the thing that takes a run down. +func TestResidueWarningSurvivesNilArguments(t *testing.T) { + warnDetectionResidue(nil, &environment.EnvironmentInfo{PBSResidual: "directory (/etc/proxmox-backup)"}) + warnDetectionResidue(logging.NewBootstrapLogger(), nil) +} From 38c804ed2e2616f4928de56bf8de05afeffb627b Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 21:04:28 +0200 Subject: [PATCH 10/15] fix: close the gaps an adversarial review found in the issue #315 work An 81-agent review of the eight commits raised 38 claims; 28 survived two skeptics each. This closes the ones that were about the code rather than about wording, plus the wording that was wrong. The one that mattered: incomplete_targets was recorded only in the collection manifest, and that manifest carries a comment saying restore never opens it. The record restore actually reads is the archive sidecar, so a dual archive that lost its PBS half still declared targets pve+pbs and cleared ValidateCompatibility against a dual host as a whole archive would, with the PBS categories offered and nothing behind them. The gap now travels in the sidecar, and DetectBackupType subtracts it: a run that lost a half ships an archive of the other half and is treated as one. Losing both halves reports unknown rather than claiming a product. Two defects in code this branch wrote: - CollectAll returned the bare context error when a cancelled run reached the system phase, dropping roleErr, so the log said "context canceled" where the phase that died had a name. Both are joined now. - The both-halves-failed error wrapped the PVE cause with %w and the PBS cause with %v, so the PBS cause was in the text but unreachable to errors.Is. Provenance: the versionless-command rung is a hit that keeps walking, and decidedBy returned the first hit, so PVESource named the probe that had failed while dpkg one rung down supplied the version. Steps that keep walking are marked Continued and skipped when naming the decider, falling back to the command when nothing else answered. The residue warning no longer declares the package absent. dpkgPackageInstalled returns false both when the stanza says not-installed and when the status file cannot be read, and under SYSTEM_ROOT_PREFIX the second is the usual case: a mount carrying /etc but not the /var holding the package database. It now reports what was observed, that nothing proved the product installed. Also: WriteManifest read c.incomplete without the lock every other access takes; a dual run that lost a half logged "collection completed"; the release note called a warning a "notice" while it demotes the run to exit 1; an orchestrator test started asking the build host what it is once DetectCurrentSystem began running real probes; TestIncompleteTargetTravelsInTheManifest asserted an in-memory field and never wrote a manifest; the PBS half of the versionless-command fix had no test; the rootPrefix safety comment still claimed both setters are bootstrap-only, which the restore delegation makes false; two comments named outcomes the code does not return; and the restore and collector docs still presented the deleted DetectCurrentSystem body and newDualRecipe as current. Three files this branch touched were not gofmt-clean. --- cmd/proxsave/main_runtime.go | 26 +++-- cmd/proxsave/main_runtime_residue_test.go | 12 ++- docs/COLLECTOR_ARCHITECTURE.md | 16 ++- docs/DEVELOPER_GUIDE.md | 1 - docs/RESTORE_TECHNICAL.md | 28 ++--- internal/backup/checksum.go | 15 ++- internal/backup/collector.go | 9 ++ internal/backup/collector_bricks.go | 1 - internal/backup/collector_dual.go | 15 ++- .../backup/collector_dual_partial_test.go | 46 ++++++-- internal/backup/collector_manifest.go | 20 ++-- internal/environment/detect.go | 49 ++++++--- internal/environment/detect_residue_test.go | 101 ++++++++++++++++++ .../orchestrator/additional_helpers_test.go | 6 ++ internal/orchestrator/backup_run_helpers.go | 28 ++--- internal/orchestrator/compatibility.go | 40 ++++++- .../compatibility_incomplete_test.go | 76 +++++++++++++ internal/orchestrator/orchestrator.go | 7 +- internal/whatsnew/registry.go | 4 +- 19 files changed, 405 insertions(+), 95 deletions(-) create mode 100644 internal/orchestrator/compatibility_incomplete_test.go diff --git a/cmd/proxsave/main_runtime.go b/cmd/proxsave/main_runtime.go index e3f3d554..a58483d1 100644 --- a/cmd/proxsave/main_runtime.go +++ b/cmd/proxsave/main_runtime.go @@ -70,15 +70,23 @@ func logDetectionProvenance(bootstrap *logging.BootstrapLogger, info *environmen warnDetectionResidue(bootstrap, info) } -// warnDetectionResidue reports, at warning, a product that left files behind without -// being installed. The trace above says the same thing, but only on a debug run, and -// the operator who needs this is the one looking at a type they did not expect: they -// see PVE where a PBS directory exists, and nothing at a normal level tells them that -// ProxSave looked at that directory and decided it does not count (issue #315). +// warnDetectionResidue reports, at warning, a product whose files are present while no +// marker proved it installed. The trace above says the same thing, but only on a debug +// run, and the operator who needs this is the one looking at a type they did not +// expect: they see PVE where a PBS directory exists, and nothing at a normal level +// tells them that ProxSave looked at that directory and decided it does not count +// (issue #315). // -// It stays quiet on a host that has the product, because the ladder returns at the -// first marker that proves an install and never reaches the residue rungs, so there -// is nothing to report on a healthy host of either kind. +// The wording stops short of declaring the package absent, which the residue does not +// prove. dpkgPackageInstalled returns false both when the stanza says not-installed and +// when the status file cannot be read at all, and under SYSTEM_ROOT_PREFIX the second +// is the common case: a mount carrying /etc but not the /var that holds the package +// database. Saying "no installed-product marker answered" is what was actually +// observed; saying "the package is not installed" would be a guess, and on a partial +// mount a wrong one about a host that has it. +// +// It stays quiet whenever a product is found, because the ladder returns at the first +// marker that proves an install and records no residue for that product. func warnDetectionResidue(bootstrap *logging.BootstrapLogger, info *environment.EnvironmentInfo) { if bootstrap == nil || info == nil { return @@ -94,7 +102,7 @@ func warnDetectionResidue(bootstrap *logging.BootstrapLogger, info *environment. if strings.TrimSpace(residue.marker) == "" { continue } - bootstrap.Warning("%s files without a %s install - %s is present but the %s package is not, so this host is collected as %s", + bootstrap.Warning("%s files without a %s install - %s is present but nothing proved %s is installed, so this host is collected as %s", residue.product, residue.product, residue.marker, residue.pkg, info.Type) } } diff --git a/cmd/proxsave/main_runtime_residue_test.go b/cmd/proxsave/main_runtime_residue_test.go index c37e2b54..c7f30c8e 100644 --- a/cmd/proxsave/main_runtime_residue_test.go +++ b/cmd/proxsave/main_runtime_residue_test.go @@ -9,9 +9,15 @@ import ( ) // TestResidueWarningStaysQuietOnAHealthyHost is the one that keeps this line useful. -// Detection returns at the first marker that proves an install and never reaches the -// residue rungs, so a host that has the product records no residue and must produce no -// warning. A line that fired on every run would be ignored by the time it mattered. +// A product that was found records no residue, so a healthy host of either kind must +// produce no warning. A line that fired on every run would be ignored by the time it +// mattered. +// +// It is the RESIDUE FIELD being empty that makes it quiet, not the ladder stopping +// early: since the versionless-command rung keeps walking, a host whose command +// answered without a version does reach the residue rungs, and detectPVE/detectPBS +// still return an empty residue because the product was proved installed. That is +// covered on the detection side by TestAVersionlessCommandStillProvesTheInstall. func TestResidueWarningStaysQuietOnAHealthyHost(t *testing.T) { for _, info := range []*environment.EnvironmentInfo{ {Type: types.ProxmoxVE}, diff --git a/docs/COLLECTOR_ARCHITECTURE.md b/docs/COLLECTOR_ARCHITECTURE.md index 136353fd..b4720120 100644 --- a/docs/COLLECTOR_ARCHITECTURE.md +++ b/docs/COLLECTOR_ARCHITECTURE.md @@ -50,13 +50,12 @@ split across files: - `newPVERecipe()` (`collector_bricks_pve.go`) - `newPBSRecipe()` (`collector_bricks_pbs.go`) - `newSystemRecipe()` (`collector_bricks_system.go`) -- `newDualRecipe()` (`collector_bricks.go`) ### Composition Rules - `newPVERecipe()` = PVE-only bricks - `newPBSRecipe()` = PBS-only bricks -- `newDualRecipe()` = PVE bricks + PBS bricks +- the dual branch runs `newPVERecipe()` and `newPBSRecipe()` as two recipes over one state - `newSystemRecipe()` = common/system bricks only `system/common` is executed once. It is not duplicated inside `dual`. @@ -125,8 +124,15 @@ user-list. ## Dual Branch -`CollectDualConfigs()` runs `newDualRecipe()` and collects both product roles in -a single backup run. +`CollectDualConfigs()` runs `newPVERecipe()` and `newPBSRecipe()` as two separate +recipes over one shared collection state, and collects both product roles in a single +backup run. + +They are separate on purpose. `runRecipe` is fail-fast, so as one concatenated recipe +an abort anywhere in the PBS half discarded the PVE payload already collected, and the +workspace holding it was deleted (issue #315). A half that fails is now recorded in +`incomplete_targets` and reported at warning; the other half is kept. Both halves +failing is still an error, because no role payload is left to keep. Important semantics: @@ -219,7 +225,7 @@ real collector flow. ## Related Files - `internal/backup/collector.go` -- `internal/backup/collector_bricks.go` (recipe machinery, brick IDs, `newDualRecipe()`) +- `internal/backup/collector_bricks.go` (recipe machinery, brick IDs) - `internal/backup/collector_bricks_pve.go` (`newPVERecipe()`) - `internal/backup/collector_bricks_pbs.go` (`newPBSRecipe()`, `newPBSUserConfigRecipe()`) - `internal/backup/collector_bricks_system.go` (`newSystemRecipe()`) diff --git a/docs/DEVELOPER_GUIDE.md b/docs/DEVELOPER_GUIDE.md index c681c333..b85c4f00 100644 --- a/docs/DEVELOPER_GUIDE.md +++ b/docs/DEVELOPER_GUIDE.md @@ -166,7 +166,6 @@ wrappers. It is built from explicit recipes and fine-grained bricks: - `newPVERecipe()` - `newPBSRecipe()` -- `newDualRecipe()` - `newSystemRecipe()` Important invariants: diff --git a/docs/RESTORE_TECHNICAL.md b/docs/RESTORE_TECHNICAL.md index 4671710b..7027a389 100644 --- a/docs/RESTORE_TECHNICAL.md +++ b/docs/RESTORE_TECHNICAL.md @@ -552,25 +552,25 @@ if err := ValidateCompatibility(systemType, backupType); err != nil { **System Detection** (`compatibility.go`): ```go func DetectCurrentSystem() SystemType { - hasPVE := fileExists("/etc/pve") || fileExists("/usr/bin/qm") || fileExists("/usr/bin/pct") - hasPBS := fileExists("/etc/proxmox-backup") || fileExists("/usr/sbin/proxmox-backup-proxy") - - switch { - case hasPVE && hasPBS: - return SystemTypeDual - case hasPVE: - return SystemTypePVE - case hasPBS: - return SystemTypePBS - default: - return SystemTypeUnknown - } + info, _ := detectEnvironment() // environment.Detect + // maps types.ProxmoxDual/VE/BS onto SystemTypeDual/PVE/PBS, unknown otherwise } ``` +Restore does not carry its own rule for what this host is: it reads the same detection +ladder the backup side uses. The rule it replaced tested `/etc/proxmox-backup` OR +`/usr/sbin/proxmox-backup-proxy`, and that second path exists on no PBS release (the +proxy is a systemd unit, not a binary on PATH), so every PBS restore decision rested on +one directory that no package owns and no removal deletes. That is the marker which +turned a PVE-only host into a dual backup in issue #315. + Restore compatibility is therefore **capability-based**, not exact-match only. -**Backup Type Detection**: +**Backup Type Detection** subtracts any role the archive was meant to carry and does +not, recorded in the sidecar manifest as `incomplete_targets`. A dual run that lost its +PBS half ships a PVE archive and is treated as one, so it reports partial compatibility +against a dual host instead of clearing the check as though nothing were missing. + ```go func DetectBackupType(manifest *backup.Manifest) SystemType { if len(manifest.ProxmoxTargets) > 0 { diff --git a/internal/backup/checksum.go b/internal/backup/checksum.go index 7b67e99c..be9169d9 100644 --- a/internal/backup/checksum.go +++ b/internal/backup/checksum.go @@ -30,10 +30,17 @@ type Manifest struct { CompressionMode string `json:"compression_mode,omitempty"` ProxmoxType string `json:"proxmox_type"` ProxmoxTargets []string `json:"proxmox_targets,omitempty"` - ProxmoxVersion string `json:"proxmox_version,omitempty"` - PVEVersion string `json:"pve_version,omitempty"` - PBSVersion string `json:"pbs_version,omitempty"` - Hostname string `json:"hostname"` + // IncompleteTargets names a role this archive was supposed to carry and does + // not, because its collection aborted while the run carried on. Absent on a + // whole archive. This is the sidecar, the record restore actually reads, so the + // gap has to be here: the collection manifest is an ExportOnly diagnostic that + // restore never opens, and a half-empty dual archive recorded only there would + // pass the compatibility check as a whole one. + IncompleteTargets []string `json:"incomplete_targets,omitempty"` + ProxmoxVersion string `json:"proxmox_version,omitempty"` + PVEVersion string `json:"pve_version,omitempty"` + PBSVersion string `json:"pbs_version,omitempty"` + Hostname string `json:"hostname"` // ServerID is the server identity of the machine that wrote the archive, the // same 16-digit value the identity file persists. Retention uses it to confirm // that an archive naming a spelling of this host's name really is this host's diff --git a/internal/backup/collector.go b/internal/backup/collector.go index 2a4d0bf7..12ba1736 100644 --- a/internal/backup/collector.go +++ b/internal/backup/collector.go @@ -497,6 +497,8 @@ func (c *Collector) CollectAll(ctx context.Context) error { c.logger.Debug("Invoking dual-role collectors (PVE + PBS recipes, shared system/common once)") if err := c.CollectDualConfigs(ctx); err != nil { roleErr = fmt.Errorf("dual collection failed: %w", err) + } else if targets := c.IncompleteTargets(); len(targets) > 0 { + c.logger.Debug("Dual-role collection finished without %s", strings.Join(targets, ", ")) } else { c.logger.Debug("Dual-role collection completed") } @@ -507,6 +509,13 @@ func (c *Collector) CollectAll(ctx context.Context) error { // Collect common system information (always collect) if err := ctx.Err(); err != nil { + // A cancelled context ends the run, but not at the cost of what already went + // wrong: returning the bare context error here dropped roleErr, so a log said + // "context canceled" where the phase that actually died was named one line + // earlier. Join keeps both reachable through errors.Is. + if roleErr != nil { + return errors.Join(roleErr, err) + } return err } c.logger.Debug("Collecting baseline system information (network/system files, commands, hardware data)") diff --git a/internal/backup/collector_bricks.go b/internal/backup/collector_bricks.go index b6287bb8..10601723 100644 --- a/internal/backup/collector_bricks.go +++ b/internal/backup/collector_bricks.go @@ -416,4 +416,3 @@ func (s *collectionState) ensurePVERuntimeInfo() *pveRuntimeInfo { } return s.pve.runtimeInfo } - diff --git a/internal/backup/collector_dual.go b/internal/backup/collector_dual.go index 1c18856d..361b7093 100644 --- a/internal/backup/collector_dual.go +++ b/internal/backup/collector_dual.go @@ -2,6 +2,7 @@ package backup import ( "context" + "errors" "fmt" ) @@ -32,7 +33,10 @@ func (c *Collector) CollectDualConfigs(ctx context.Context) error { switch { case pveErr != nil && pbsErr != nil: - return fmt.Errorf("both halves failed: PVE: %w; PBS: %v", pveErr, pbsErr) + // Both causes are wrapped, not just the first. %w on one and %v on the other + // put the PBS text in the message while leaving it unreachable to errors.Is, + // so a caller testing for a sentinel saw only half the failure. + return fmt.Errorf("both halves failed: %w", errors.Join(pveErr, pbsErr)) case pveErr != nil: c.noteIncompleteTarget("pve", pveErr) case pbsErr != nil: @@ -67,6 +71,15 @@ func (c *Collector) noteIncompleteTarget(target string, cause error) { c.logger.Warning("Collection: this backup carries the other role and the system payload, and is marked incomplete for %s", target) } +// incompleteSnapshot copies the recorded gaps under the same lock every other access +// to the slice takes. WriteManifest read it bare, which is a race the moment anything +// records a gap off the collection goroutine. +func (c *Collector) incompleteSnapshot() []incompleteTarget { + c.statsMu.Lock() + defer c.statsMu.Unlock() + return append([]incompleteTarget(nil), c.incomplete...) +} + // IncompleteTargets returns the roles whose collection did not finish, for a caller // that reports run status. Empty on a whole backup. func (c *Collector) IncompleteTargets() []string { diff --git a/internal/backup/collector_dual_partial_test.go b/internal/backup/collector_dual_partial_test.go index 2d3e2666..a673fc46 100644 --- a/internal/backup/collector_dual_partial_test.go +++ b/internal/backup/collector_dual_partial_test.go @@ -2,6 +2,7 @@ package backup import ( "context" + "encoding/json" "errors" "io" "os" @@ -17,6 +18,14 @@ import ( // configuration directories the caller asks for is created; the other is absent, which // is how each half is made to fail at its validate brick. func dualCollector(t *testing.T, withPVE, withPBS bool) *Collector { + t.Helper() + return dualCollectorWithDryRun(t, withPVE, withPBS, true) +} + +// dualCollectorWithDryRun is the same fixture with the dry-run switch exposed: the +// manifest is only written on a real run, so a test that reads it back off disk needs +// dryRun=false. +func dualCollectorWithDryRun(t *testing.T, withPVE, withPBS, dryRun bool) *Collector { t.Helper() logger := logging.New(types.LogLevelError, false) logger.SetOutput(io.Discard) @@ -49,7 +58,7 @@ func dualCollector(t *testing.T, withPVE, withPBS bool) *Collector { return []byte("[]"), nil } - return NewCollectorWithDeps(logger, cfg, t.TempDir(), types.ProxmoxDual, true, deps) + return NewCollectorWithDeps(logger, cfg, t.TempDir(), types.ProxmoxDual, dryRun, deps) } // TestDualKeepsThePVEHalfWhenPBSFails is the loss issue #315 caused. The two halves @@ -101,22 +110,39 @@ func TestDualFailsWhenBothHalvesFail(t *testing.T) { } // TestIncompleteTargetTravelsInTheManifest: a log can be rotated away or never read, -// and the archive then looks whole. The manifest is what goes with it. +// and the archive then looks whole. This writes the manifest and reads it back off +// disk, because an earlier version of this test asserted the in-memory field and +// claimed the manifest in its name without ever producing one. func TestIncompleteTargetTravelsInTheManifest(t *testing.T) { - collector := dualCollector(t, true, false) + collector := dualCollectorWithDryRun(t, true, false, false) if err := collector.CollectAll(context.Background()); err != nil { t.Fatalf("CollectAll: %v", err) } + if err := collector.WriteManifest("host.example"); err != nil { + t.Fatalf("WriteManifest: %v", err) + } - if len(collector.incomplete) != 1 { - t.Fatalf("collector.incomplete = %v, want one entry", collector.incomplete) + path := filepath.Join(collector.tempDir, "manifest.json") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + var written BackupManifest + if err := json.Unmarshal(data, &written); err != nil { + t.Fatalf("unmarshal manifest: %v", err) + } + + if len(written.Incomplete) != 1 { + t.Fatalf("manifest incomplete_targets = %+v, want one entry", written.Incomplete) + } + if written.Incomplete[0].Target != "pbs" { + t.Fatalf("incomplete target = %q, want pbs", written.Incomplete[0].Target) } - entry := collector.incomplete[0] - if entry.Target != "pbs" { - t.Fatalf("incomplete target = %q, want pbs", entry.Target) + if strings.TrimSpace(written.Incomplete[0].Reason) == "" { + t.Fatal("the manifest says a half is missing without saying why") } - if strings.TrimSpace(entry.Reason) == "" { - t.Fatal("the recorded reason is empty: the manifest would say a half is missing without saying why") + if !strings.Contains(string(data), "incomplete_targets") { + t.Fatal("the JSON key is absent: a reader of the archive cannot see the gap") } } diff --git a/internal/backup/collector_manifest.go b/internal/backup/collector_manifest.go index f25b21d8..812f9581 100644 --- a/internal/backup/collector_manifest.go +++ b/internal/backup/collector_manifest.go @@ -31,18 +31,18 @@ type ManifestEntry struct { // record of the shipped payload is the archive sidecar (.sha256 and // .manifest.json), computed after the archive is built. type BackupManifest struct { - CreatedAt time.Time `json:"created_at"` - Hostname string `json:"hostname"` - ProxmoxType string `json:"proxmox_type"` - ProxmoxTargets []string `json:"proxmox_targets,omitempty"` + CreatedAt time.Time `json:"created_at"` + Hostname string `json:"hostname"` + ProxmoxType string `json:"proxmox_type"` + ProxmoxTargets []string `json:"proxmox_targets,omitempty"` // Incomplete names any role whose collection aborted while the run carried on. // Absent on a whole backup. It is what stops a partial archive from reading like // a complete one once the run log is gone. - Incomplete []incompleteTarget `json:"incomplete_targets,omitempty"` - PBSConfigs map[string]ManifestEntry `json:"pbs_configs,omitempty"` - PVEConfigs map[string]ManifestEntry `json:"pve_configs,omitempty"` - SystemFiles map[string]ManifestEntry `json:"system_files,omitempty"` - Stats ManifestStats `json:"stats"` + Incomplete []incompleteTarget `json:"incomplete_targets,omitempty"` + PBSConfigs map[string]ManifestEntry `json:"pbs_configs,omitempty"` + PVEConfigs map[string]ManifestEntry `json:"pve_configs,omitempty"` + SystemFiles map[string]ManifestEntry `json:"system_files,omitempty"` + Stats ManifestStats `json:"stats"` } // ManifestStats contains summary statistics for the manifest @@ -62,7 +62,7 @@ func (c *Collector) WriteManifest(hostname string) error { Hostname: hostname, ProxmoxType: string(c.proxType), ProxmoxTargets: append([]string(nil), c.proxType.Targets()...), - Incomplete: append([]incompleteTarget(nil), c.incomplete...), + Incomplete: c.incompleteSnapshot(), PBSConfigs: c.pbsManifest, PVEConfigs: c.pveManifest, SystemFiles: c.systemManifest, diff --git a/internal/environment/detect.go b/internal/environment/detect.go index ccf7896e..e1c78908 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -115,11 +115,13 @@ var ( // readFileFunc above. // // EXACTLY TWO functions set it, each for the duration of one call and each - // restoring the previous value in a defer: DetectWith and MarkerSnapshot. Both - // are called from the process bootstrap, which is sequential and runs before any - // goroutine of this program exists, so the two never overlap. That is the whole - // safety argument - there is no lock - and it holds only while the call sites stay - // where they are. + // restoring the previous value in a defer: DetectWith and MarkerSnapshot. + // + // Neither is bootstrap-only any more: orchestrator.DetectCurrentSystem calls + // environment.Detect on the restore path, so a set/restore pair now happens while + // a restore is being planned as well. That is still safe for the same reason it + // always was, and only for that reason: every call site is on the one goroutine + // its run uses, so no two overlap. There is no lock. // // A third setter, or either of these two reached from a goroutine, breaks it: one // call would inspect paths under another call's prefix and return a snapshot or a @@ -201,13 +203,17 @@ const ( // type". type DetectionStep struct { Product string // productPVE or productPBS - Marker string // command, version-file, dpkg, cluster-db, binary, share-dir, directory + Marker string // command, version-file, dpkg, cluster-db, binary, share-dir, apt-source, directory Target string // path(s) or command consulted Hit bool Skipped bool // probe not run at all (command probes under a host prefix) Residual bool // marker found, but it does not prove the product is installed - Version string - Note string // why a probe was skipped, or what the residue means + // Continued marks a hit that proved the product installed WITHOUT ending the + // ladder, because it carried no version and a later rung may still have one. It + // is what keeps decidedBy naming the rung that actually ended the walk. + Continued bool + Version string + Note string // why a probe was skipped, or what the residue means } // String renders the step as the single line a debug log carries. @@ -264,10 +270,22 @@ func (t *detectionTrace) skip(product, marker, target, note string) { } // decidedBy names the marker that ended the ladder for product; empty when none did. +// +// A Continued step is skipped on purpose. The versionless-command rung is a hit that +// keeps walking, and returning the first hit named it as the decider while the version +// came from dpkg one rung further down, so the provenance line pointed a diagnosing +// operator at the probe that had failed. func (t *detectionTrace) decidedBy(product string) string { if t == nil { return "" } + for _, step := range t.steps { + if step.Product == product && step.Hit && !step.Continued { + return fmt.Sprintf("%s (%s)", step.Marker, step.Target) + } + } + // Every hit kept walking: the command proved the install and no later marker + // answered, so that rung is the whole provenance there is. for _, step := range t.steps { if step.Product == product && step.Hit { return fmt.Sprintf("%s (%s)", step.Marker, step.Target) @@ -462,7 +480,7 @@ func detectPVE(trace *detectionTrace) (string, bool, string) { return version, true, "" case markerInstalledNoVersion: installedWithoutVersion = true - trace.add(DetectionStep{Product: productPVE, Marker: "command", Target: "pveversion", Hit: true, + trace.add(DetectionStep{Product: productPVE, Marker: "command", Target: "pveversion", Hit: true, Continued: true, Note: "the command is installed but gave no version; looking for one further down"}) default: trace.miss(productPVE, "command", "pveversion") @@ -557,7 +575,7 @@ func detectPBS(trace *detectionTrace) (string, bool, string) { return version, true, "" case markerInstalledNoVersion: installedWithoutVersion = true - trace.add(DetectionStep{Product: productPBS, Marker: "command", Target: "proxmox-backup-manager", Hit: true, + trace.add(DetectionStep{Product: productPBS, Marker: "command", Target: "proxmox-backup-manager", Hit: true, Continued: true, Note: "the command is installed but gave no version; looking for one further down"}) default: trace.miss(productPBS, "command", "proxmox-backup-manager") @@ -690,10 +708,10 @@ func dpkgStanzaField(stanza, key string) string { // // That failure is not rare. pveversion takes 4.4 to 5.2 seconds on the lab host and // commandTimeout is 5, so it times out intermittently on nothing more unusual than a -// busy node. The caller uses markerInstalled to stop the ladder and markerResidual to -// carry on, so a run that got the binary but not the version keeps walking and lets -// dpkg supply it, instead of settling for "unknown" with the real version one rung -// further down. +// busy node. The caller stops the ladder on markerInstalled and keeps walking on +// markerInstalledNoVersion, so a run that got the binary but not the version lets dpkg +// supply it instead of settling for "unknown" with the real version one rung further +// down. func detectPVEViaCommand() (string, markerOutcome) { cmdPath, err := lookPathFunc("pveversion") if err != nil { @@ -780,9 +798,6 @@ func detectPBSViaVersionFile() (string, markerOutcome) { return "", markerResidual } - - - func extendPath() { currentPath := os.Getenv("PATH") pathSet := make(map[string]struct{}) diff --git a/internal/environment/detect_residue_test.go b/internal/environment/detect_residue_test.go index 60a047a9..b3650375 100644 --- a/internal/environment/detect_residue_test.go +++ b/internal/environment/detect_residue_test.go @@ -324,3 +324,104 @@ func TestAVersionlessCommandStillProvesTheInstall(t *testing.T) { t.Fatalf("Type = %v, want ProxmoxVE", info.Type) } } + +// TestProvenanceNamesTheRungThatDecided: the versionless-command rung is a hit that +// keeps walking, and decidedBy returning the first hit named it as the decider while +// dpkg one rung down supplied the version. An operator reading "PVE decided by command +// (pveversion)" above a step list showing that probe produced nothing was pointed at +// the wrong marker. +func TestProvenanceNamesTheRungThatDecided(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + + dpkgStatus := filepath.Join(tmp, "dpkg-status") + writeFile(t, dpkgStatus, dpkgStanza("pve-manager", "9.2.18")+dpkgStanza("proxmox-backup-server", "4.2.0-1")) + setValue(t, &dpkgStatusFile, dpkgStatus) + + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + switch cmd { + case "pveversion": + return "/usr/bin/pveversion", nil + case "proxmox-backup-manager": + return "/usr/sbin/proxmox-backup-manager", nil + } + return "", errors.New("not found") + }) + setValue(t, &runCommandFunc, func(string, ...string) (string, error) { + return "", errors.New("command timed out") + }) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxDual { + t.Fatalf("Type = %v, want ProxmoxDual", info.Type) + } + if !strings.HasPrefix(info.PVESource, "dpkg pve-manager") { + t.Fatalf("PVESource = %q, want the dpkg rung that supplied the version", info.PVESource) + } + if !strings.HasPrefix(info.PBSSource, "dpkg proxmox-backup-server") { + t.Fatalf("PBSSource = %q, want the dpkg rung that supplied the version", info.PBSSource) + } +} + +// TestProvenanceFallsBackToTheCommandWhenNothingElseAnswers: with no later marker, the +// rung that kept walking is the whole provenance there is, and an empty source line +// would be worse than naming it. +func TestProvenanceFallsBackToTheCommandWhenNothingElseAnswers(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + if cmd == "pveversion" { + return "/usr/bin/pveversion", nil + } + return "", errors.New("not found") + }) + setValue(t, &runCommandFunc, func(string, ...string) (string, error) { + return "no version here", nil + }) + + info, _ := detectEnvironmentInfo() + if info.Type != types.ProxmoxVE { + t.Fatalf("Type = %v, want ProxmoxVE", info.Type) + } + if !strings.HasPrefix(info.PVESource, "command (") { + t.Fatalf("PVESource = %q, want the command rung named as the only provenance", info.PVESource) + } +} + +// TestThePBSHalfOfTheVersionlessCommandFix: the PVE half had a test and the PBS half +// had none, so the symmetry was asserted nowhere. +func TestThePBSHalfOfTheVersionlessCommandFix(t *testing.T) { + tmp := t.TempDir() + setValue(t, &additionalPaths, []string{}) + nullFilesystemMarkerSeams(t, tmp) + + dpkgStatus := filepath.Join(tmp, "dpkg-status") + writeFile(t, dpkgStatus, dpkgStanza("proxmox-backup-server", "4.2.0-1")) + setValue(t, &dpkgStatusFile, dpkgStatus) + + setValue(t, &lookPathFunc, func(cmd string) (string, error) { + if cmd == "proxmox-backup-manager" { + return "/usr/sbin/proxmox-backup-manager", nil + } + return "", errors.New("not found") + }) + setValue(t, &runCommandFunc, func(string, ...string) (string, error) { + return "", errors.New("command proxmox-backup-manager timed out") + }) + + info, err := detectEnvironmentInfo() + if err != nil { + t.Fatalf("detectEnvironmentInfo: %v", err) + } + if info.Type != types.ProxmoxBS { + t.Fatalf("Type = %v, want ProxmoxBS", info.Type) + } + if info.PBSVersion != "4.2.0-1" { + t.Fatalf("PBSVersion = %q, want 4.2.0-1 recovered from dpkg", info.PBSVersion) + } +} diff --git a/internal/orchestrator/additional_helpers_test.go b/internal/orchestrator/additional_helpers_test.go index 8cbdcf41..cb09bfa2 100644 --- a/internal/orchestrator/additional_helpers_test.go +++ b/internal/orchestrator/additional_helpers_test.go @@ -1747,6 +1747,12 @@ func TestDispatchNotificationsAndLogsSkipsWithNoLog(t *testing.T) { } func TestCheckSystemRequirementsNoPanic(t *testing.T) { + // DetectCurrentSystem now walks the real detection ladder, which runs pveversion + // and proxmox-backup-manager with a 5s timeout each. Left unstubbed this test asks + // the build host what it is, so it both slows down and answers differently on a + // developer laptop and on the PVE box the suite is usually run on. + stubDetection(t, &environment.EnvironmentInfo{Type: types.ProxmoxBS}, nil) + // manifest nil CheckSystemRequirements(nil) // just ensure no panic diff --git a/internal/orchestrator/backup_run_helpers.go b/internal/orchestrator/backup_run_helpers.go index f3886437..6b17c3f9 100644 --- a/internal/orchestrator/backup_run_helpers.go +++ b/internal/orchestrator/backup_run_helpers.go @@ -107,6 +107,7 @@ func (o *Orchestrator) applyBackupCollectionStats(stats *BackupStats, collStats stats.FilesIncluded = int(collStats.FilesProcessed) stats.FilesMissing = int(collStats.FilesNotFound) stats.UncompressedSize = collStats.BytesCollected + stats.IncompleteTargets = collector.IncompleteTargets() if stats.ProxmoxType.SupportsPVE() { stats.ClusterMode = standaloneClusterMode(collector) } @@ -329,19 +330,20 @@ func (o *Orchestrator) newArchiveManifest(stats *BackupStats, archivePath, check return nil, err } return &backup.Manifest{ - ArchivePath: archivePath, - ArchiveSize: stats.ArchiveSize, - SHA256: checksum, - CreatedAt: stats.Timestamp, - CompressionType: string(stats.Compression), - CompressionLevel: stats.CompressionLevel, - CompressionMode: stats.CompressionMode, - ProxmoxType: string(stats.ProxmoxType), - ProxmoxTargets: append([]string(nil), stats.ProxmoxTargets...), - ProxmoxVersion: stats.ProxmoxVersion, - PVEVersion: stats.PVEVersion, - PBSVersion: stats.PBSVersion, - Hostname: stats.Hostname, + ArchivePath: archivePath, + ArchiveSize: stats.ArchiveSize, + SHA256: checksum, + CreatedAt: stats.Timestamp, + CompressionType: string(stats.Compression), + CompressionLevel: stats.CompressionLevel, + CompressionMode: stats.CompressionMode, + ProxmoxType: string(stats.ProxmoxType), + ProxmoxTargets: append([]string(nil), stats.ProxmoxTargets...), + IncompleteTargets: append([]string(nil), stats.IncompleteTargets...), + ProxmoxVersion: stats.ProxmoxVersion, + PVEVersion: stats.PVEVersion, + PBSVersion: stats.PBSVersion, + Hostname: stats.Hostname, // The one place the run's server identity reaches the archives. It is what // lets a later run recognise this archive as its own after the machine stops // resolving the name stamped beside it (discussion #292). Empty when this diff --git a/internal/orchestrator/compatibility.go b/internal/orchestrator/compatibility.go index d14736ca..89ba1e31 100644 --- a/internal/orchestrator/compatibility.go +++ b/internal/orchestrator/compatibility.go @@ -76,14 +76,26 @@ func DetectCurrentSystem() SystemType { } } -// DetectBackupType detects the type of backup from manifest +// DetectBackupType detects the type of backup from manifest. +// +// A role the archive was supposed to carry and does not is subtracted first. The +// targets field records what the run SET OUT to collect; incomplete_targets records +// what it failed to bring back. A dual run that lost its PBS half ships a PVE +// archive, and calling it dual would let it clear ValidateCompatibility against a +// dual host as though nothing were missing, with the PBS categories offered and +// nothing behind them. func DetectBackupType(manifest *backup.Manifest) SystemType { if manifest == nil { return SystemTypeUnknown } - if len(manifest.ProxmoxTargets) > 0 { - return parseSystemTargets(manifest.ProxmoxTargets) + if targets := completedTargets(manifest); len(targets) > 0 { + return parseSystemTargets(targets) + } + if len(manifest.ProxmoxTargets) > 0 && len(manifest.IncompleteTargets) > 0 { + // Every declared target failed. The archive carries no role payload at all, + // so it is not a backup of either product. + return SystemTypeUnknown } // Check ProxmoxType field if present @@ -108,6 +120,28 @@ func DetectBackupType(manifest *backup.Manifest) SystemType { return SystemTypeUnknown } +// completedTargets is the declared target list with every incomplete role removed. +// Matching is case-insensitive and trimmed because the two lists are written by +// different code paths: targets by ProxmoxType.Targets(), incomplete by the +// collector naming the recipe that failed. +func completedTargets(manifest *backup.Manifest) []string { + if len(manifest.ProxmoxTargets) == 0 { + return nil + } + missing := make(map[string]struct{}, len(manifest.IncompleteTargets)) + for _, target := range manifest.IncompleteTargets { + missing[strings.ToLower(strings.TrimSpace(target))] = struct{}{} + } + kept := make([]string, 0, len(manifest.ProxmoxTargets)) + for _, target := range manifest.ProxmoxTargets { + if _, gone := missing[strings.ToLower(strings.TrimSpace(target))]; gone { + continue + } + kept = append(kept, target) + } + return kept +} + func parseSystemTypeString(value string) SystemType { normalized := strings.ToLower(strings.TrimSpace(value)) switch { diff --git a/internal/orchestrator/compatibility_incomplete_test.go b/internal/orchestrator/compatibility_incomplete_test.go new file mode 100644 index 00000000..6426c6a4 --- /dev/null +++ b/internal/orchestrator/compatibility_incomplete_test.go @@ -0,0 +1,76 @@ +package orchestrator + +import ( + "strings" + "testing" + + "github.com/tis24dev/proxsave/internal/backup" +) + +// TestAPartialDualArchiveIsNotADualBackup is the restore half of the partial-archive +// contract. The collection manifest is an ExportOnly diagnostic restore never opens, +// so recording the gap only there let a dual archive that lost its PBS half clear the +// compatibility check against a dual host as though nothing were missing: PBS +// categories offered, nothing behind them. +func TestAPartialDualArchiveIsNotADualBackup(t *testing.T) { + manifest := &backup.Manifest{ + ProxmoxType: "dual", + ProxmoxTargets: []string{"pve", "pbs"}, + IncompleteTargets: []string{"pbs"}, + } + + if got := DetectBackupType(manifest); got != SystemTypePVE { + t.Fatalf("DetectBackupType() = %s, want %s: the archive carries no PBS payload", got, SystemTypePVE) + } + + err := ValidateCompatibility(SystemTypeDual, DetectBackupType(manifest)) + if err == nil { + t.Fatal("ValidateCompatibility passed a half-empty archive as a whole one") + } + if !strings.Contains(err.Error(), "partial compatibility") { + t.Fatalf("ValidateCompatibility error = %v, want it to report partial compatibility", err) + } +} + +// TestAWholeDualArchiveIsStillDual: the subtraction must not fire on a complete run, +// or every dual backup would restore as half of itself. +func TestAWholeDualArchiveIsStillDual(t *testing.T) { + manifest := &backup.Manifest{ProxmoxType: "dual", ProxmoxTargets: []string{"pve", "pbs"}} + + if got := DetectBackupType(manifest); got != SystemTypeDual { + t.Fatalf("DetectBackupType() = %s, want %s", got, SystemTypeDual) + } + if err := ValidateCompatibility(SystemTypeDual, SystemTypeDual); err != nil { + t.Fatalf("ValidateCompatibility = %v, want a whole dual archive to pass", err) + } +} + +// TestAnArchiveThatLostEveryRoleIsUnknown: with both halves gone there is no role +// payload at all, and claiming either product would send restore looking for files +// the archive does not hold. +func TestAnArchiveThatLostEveryRoleIsUnknown(t *testing.T) { + manifest := &backup.Manifest{ + ProxmoxType: "dual", + ProxmoxTargets: []string{"pve", "pbs"}, + IncompleteTargets: []string{"pve", "pbs"}, + } + + if got := DetectBackupType(manifest); got != SystemTypeUnknown { + t.Fatalf("DetectBackupType() = %s, want %s", got, SystemTypeUnknown) + } +} + +// TestIncompleteTargetMatchingIgnoresCaseAndSpace: the two lists are written by +// different code paths, ProxmoxType.Targets() and the collector naming the failed +// recipe, so the match cannot depend on them agreeing on spelling. +func TestIncompleteTargetMatchingIgnoresCaseAndSpace(t *testing.T) { + manifest := &backup.Manifest{ + ProxmoxType: "dual", + ProxmoxTargets: []string{"pve", "pbs"}, + IncompleteTargets: []string{" PBS "}, + } + + if got := DetectBackupType(manifest); got != SystemTypePVE { + t.Fatalf("DetectBackupType() = %s, want %s", got, SystemTypePVE) + } +} diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 90f9cf9d..b2e3bb0e 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -56,8 +56,11 @@ func (e *EarlyErrorState) HasError() bool { // BackupStats contains statistics from backup operations type BackupStats struct { - Hostname string - ProxmoxType types.ProxmoxType + Hostname string + ProxmoxType types.ProxmoxType + // IncompleteTargets names roles whose collection aborted while the run carried + // on, so the archive ships without them. Empty on a whole backup. + IncompleteTargets []string ProxmoxTargets []string ProxmoxVersion string PVEVersion string diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index 036dab13..f05e6d6c 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -205,14 +205,14 @@ var notes = []Note{ Version: "0.39.0", Lines: []string{ "A PVE host with leftover proxmox-backup files is no longer taken for a PBS server, and its backup runs again", - "A run says when a product left files behind without being installed, and names the file it found", + "A run warns when a product left files behind without being installed; that warning makes the run exit 1", "Restore reads the host type from the same check the backup uses, instead of keeping a weaker one of its own", "On a PVE plus PBS host, one role failing no longer discards the other role and the shared system payload", "A slow pveversion no longer costs the run its version number: the version is read from the package instead", }, Actions: []string{ "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", - "The leftover directories are still reported: remove them, or install PBS, only if you want the notice to stop", + "To clear the leftover warning and its exit 1, remove the reported directory or install the product it belonged to", "An archive taken before this fix is labelled dual, so restoring it on the corrected host reports partial match", "A PVE host with leftover PBS files no longer offers PBS restore categories and no longer stops PBS services", "A dual backup that lost one role exits with the warning code and names the missing role in its manifest", From 7c76b5eb8f1eecd0aee4ec43bf18a8c5765c1630 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 16 Sep 2026 21:20:37 +0200 Subject: [PATCH 11/15] fix: report detection residue at info, so leftovers stop costing a run its exit code A residue is recorded only when a product was NOT proved installed, and enumerating every mount shape with both products installed shows that leaves exactly two situations, never a third: - the verdict is pve, pbs or dual: the product is genuinely absent, the backup is complete and correct, and the leftovers are untidy filesystem rather than a fault. - the verdict is unknown: a real fault, and it already carries three warnings that decide the exit code between them - the detection error, which now names the residue itself, the host-backup mount warning, and the collector reporting that it is collecting generic system info only. There is no mount shape that yields a confident wrong type alongside a residue, and the reason is structural: /var/lib/dpkg/status is one file covering both products, so it either proves both or neither. The asymmetry that would make a residue the only hint of a missed half cannot arise. So warning level bought nothing and cost plenty. It pinned an otherwise healthy host at exit 1 on every run over leftovers its operator often cannot delete, since /var/lib/proxmox-backup belongs to the PVE file-restore stack: a nightly monitor gating on the exit code would alarm forever on a node whose backups are fine. The host in issue #315 is exactly that host. The test now pins the LEVEL rather than the line count, through ReplayConsoleSince, which replays warning and worse only: putting Warning back makes it fail. Verified by doing precisely that before committing. The release note no longer promises the exit 1 it was describing, and warnDetectionResidue is renamed reportDetectionResidue because it no longer warns. --- cmd/proxsave/main_runtime.go | 43 +++++++----- cmd/proxsave/main_runtime_residue_test.go | 80 +++++++++++++++++++---- internal/whatsnew/registry.go | 4 +- 3 files changed, 94 insertions(+), 33 deletions(-) diff --git a/cmd/proxsave/main_runtime.go b/cmd/proxsave/main_runtime.go index a58483d1..27a8c352 100644 --- a/cmd/proxsave/main_runtime.go +++ b/cmd/proxsave/main_runtime.go @@ -67,27 +67,34 @@ func logDetectionProvenance(bootstrap *logging.BootstrapLogger, info *environmen for _, step := range info.Steps { bootstrap.Debug("Detection probe: %s", step) } - warnDetectionResidue(bootstrap, info) + reportDetectionResidue(bootstrap, info) } -// warnDetectionResidue reports, at warning, a product whose files are present while no -// marker proved it installed. The trace above says the same thing, but only on a debug -// run, and the operator who needs this is the one looking at a type they did not -// expect: they see PVE where a PBS directory exists, and nothing at a normal level -// tells them that ProxSave looked at that directory and decided it does not count -// (issue #315). +// reportDetectionResidue reports a product whose files are present while no marker +// proved it installed. The trace above says the same thing, but only on a debug run, +// and the operator who needs this is the one looking at a type they did not expect: +// they see PVE where a PBS directory exists, and nothing at a normal level tells them +// that ProxSave looked at that directory and decided it does not count (issue #315). // -// The wording stops short of declaring the package absent, which the residue does not -// prove. dpkgPackageInstalled returns false both when the stanza says not-installed and -// when the status file cannot be read at all, and under SYSTEM_ROOT_PREFIX the second -// is the common case: a mount carrying /etc but not the /var that holds the package -// database. Saying "no installed-product marker answered" is what was actually -// observed; saying "the package is not installed" would be a guess, and on a partial -// mount a wrong one about a host that has it. +// INFO, not warning, and the level is the point. A residue is recorded only when a +// product was NOT proved installed, which leaves exactly two situations, and the +// mount-shape matrix confirms there is no third: // -// It stays quiet whenever a product is found, because the ladder returns at the first -// marker that proves an install and records no residue for that product. -func warnDetectionResidue(bootstrap *logging.BootstrapLogger, info *environment.EnvironmentInfo) { +// - the type came out pve, pbs or dual: the product is genuinely absent, the backup +// is complete and correct, and the leftovers are untidy filesystem, not a fault. +// Warning here pinned such a host at exit 1 on every run, for something its +// operator often cannot remove (/var/lib/proxmox-backup belongs to the PVE +// file-restore stack), so a nightly monitor would alarm forever on a healthy node. +// - the type came out unknown: that IS a fault, and it already carries three +// warnings that decide the exit code between them - the detection error here, +// which now names the residue, the host-backup mount warning, and the collector +// saying it is collecting generic system info only. A fourth would be noise. +// +// The wording also stops short of declaring the package absent, which the residue does +// not prove: dpkgPackageInstalled returns false both when the stanza says +// not-installed and when the status file cannot be read, and under SYSTEM_ROOT_PREFIX +// the second is the common case. +func reportDetectionResidue(bootstrap *logging.BootstrapLogger, info *environment.EnvironmentInfo) { if bootstrap == nil || info == nil { return } @@ -102,7 +109,7 @@ func warnDetectionResidue(bootstrap *logging.BootstrapLogger, info *environment. if strings.TrimSpace(residue.marker) == "" { continue } - bootstrap.Warning("%s files without a %s install - %s is present but nothing proved %s is installed, so this host is collected as %s", + bootstrap.Info("%s files without a %s install - %s is present but nothing proved %s is installed, so this host is collected as %s", residue.product, residue.product, residue.marker, residue.pkg, info.Type) } } diff --git a/cmd/proxsave/main_runtime_residue_test.go b/cmd/proxsave/main_runtime_residue_test.go index c7f30c8e..2260eada 100644 --- a/cmd/proxsave/main_runtime_residue_test.go +++ b/cmd/proxsave/main_runtime_residue_test.go @@ -1,6 +1,8 @@ package main import ( + "os" + "strings" "testing" "github.com/tis24dev/proxsave/internal/environment" @@ -8,17 +10,69 @@ import ( "github.com/tis24dev/proxsave/internal/types" ) -// TestResidueWarningStaysQuietOnAHealthyHost is the one that keeps this line useful. -// A product that was found records no residue, so a healthy host of either kind must -// produce no warning. A line that fired on every run would be ignored by the time it -// mattered. +// replayedWarnings returns what ReplayConsoleSince prints for entries recorded after +// mark. That method replays warning and worse only, so an empty result is proof the +// entries in between were below warning. +func replayedWarnings(t *testing.T, bootstrap *logging.BootstrapLogger, mark int) string { + t.Helper() + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stderr = w + done := make(chan string, 1) + go func() { + var sb strings.Builder + buf := make([]byte, 4096) + for { + n, err := r.Read(buf) + sb.Write(buf[:n]) + if err != nil { + break + } + } + done <- sb.String() + }() + bootstrap.ReplayConsoleSince(mark) + _ = w.Close() + os.Stderr = old + return <-done +} + +// TestResidueIsReportedBelowWarning pins the level, not just the presence of the line. +// A residue is recorded only when a product was NOT proved installed, and that leaves +// two cases: a correct verdict, where the product is genuinely absent and the backup is +// complete, or an unknown verdict, which already carries its own warnings. Reporting +// this at warning pinned an otherwise healthy host at exit 1 on every single run, for +// leftovers its operator often cannot delete. +func TestResidueIsReportedBelowWarning(t *testing.T) { + bootstrap := logging.NewBootstrapLogger() + mark := bootstrap.EntryCount() + + reportDetectionResidue(bootstrap, &environment.EnvironmentInfo{ + Type: types.ProxmoxVE, + PBSResidual: "directory (/etc/proxmox-backup)", + }) + + if got := bootstrap.EntryCount() - mark; got != 1 { + t.Fatalf("recorded %d entries, want 1", got) + } + if replayed := replayedWarnings(t, bootstrap, mark); replayed != "" { + t.Fatalf("the residue line replayed as warning-or-worse, so it still promotes a clean run off exit 0:\n%s", replayed) + } +} + +// TestResidueStaysQuietWhenAProductWasFound: a product that was found records no +// residue, so a healthy host of either kind must produce no line at all. A message that +// appeared on every run would be ignored by the time it mattered. // // It is the RESIDUE FIELD being empty that makes it quiet, not the ladder stopping // early: since the versionless-command rung keeps walking, a host whose command // answered without a version does reach the residue rungs, and detectPVE/detectPBS // still return an empty residue because the product was proved installed. That is // covered on the detection side by TestAVersionlessCommandStillProvesTheInstall. -func TestResidueWarningStaysQuietOnAHealthyHost(t *testing.T) { +func TestResidueStaysQuietWhenAProductWasFound(t *testing.T) { for _, info := range []*environment.EnvironmentInfo{ {Type: types.ProxmoxVE}, {Type: types.ProxmoxBS}, @@ -27,17 +81,17 @@ func TestResidueWarningStaysQuietOnAHealthyHost(t *testing.T) { } { bootstrap := logging.NewBootstrapLogger() before := bootstrap.EntryCount() - warnDetectionResidue(bootstrap, info) + reportDetectionResidue(bootstrap, info) if got := bootstrap.EntryCount() - before; got != 0 { t.Fatalf("type %s with residual %q logged %d entries, want silence", info.Type, info.PVEResidual, got) } } } -// TestResidueWarningFiresOncePerProduct: the host in issue #315 has PBS residue and no +// TestResidueIsReportedOncePerProduct: the host in issue #315 has PBS residue and no // PBS, and the operator looking at a type they did not expect never sees the trace, // which is debug only. -func TestResidueWarningFiresOncePerProduct(t *testing.T) { +func TestResidueIsReportedOncePerProduct(t *testing.T) { for _, tc := range []struct { name string info *environment.EnvironmentInfo @@ -61,7 +115,7 @@ func TestResidueWarningFiresOncePerProduct(t *testing.T) { t.Run(tc.name, func(t *testing.T) { bootstrap := logging.NewBootstrapLogger() before := bootstrap.EntryCount() - warnDetectionResidue(bootstrap, tc.info) + reportDetectionResidue(bootstrap, tc.info) if got := bootstrap.EntryCount() - before; got != tc.want { t.Fatalf("logged %d entries, want %d", got, tc.want) } @@ -69,9 +123,9 @@ func TestResidueWarningFiresOncePerProduct(t *testing.T) { } } -// TestResidueWarningSurvivesNilArguments: it runs on the bootstrap path, before the +// TestResidueReportSurvivesNilArguments: it runs on the bootstrap path, before the // main logger exists, and must not be the thing that takes a run down. -func TestResidueWarningSurvivesNilArguments(t *testing.T) { - warnDetectionResidue(nil, &environment.EnvironmentInfo{PBSResidual: "directory (/etc/proxmox-backup)"}) - warnDetectionResidue(logging.NewBootstrapLogger(), nil) +func TestResidueReportSurvivesNilArguments(t *testing.T) { + reportDetectionResidue(nil, &environment.EnvironmentInfo{PBSResidual: "directory (/etc/proxmox-backup)"}) + reportDetectionResidue(logging.NewBootstrapLogger(), nil) } diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index f05e6d6c..d93c0b6d 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -205,14 +205,14 @@ var notes = []Note{ Version: "0.39.0", Lines: []string{ "A PVE host with leftover proxmox-backup files is no longer taken for a PBS server, and its backup runs again", - "A run warns when a product left files behind without being installed; that warning makes the run exit 1", + "A run says when a product left files behind without being installed, and names the file it found", "Restore reads the host type from the same check the backup uses, instead of keeping a weaker one of its own", "On a PVE plus PBS host, one role failing no longer discards the other role and the shared system payload", "A slow pveversion no longer costs the run its version number: the version is read from the package instead", }, Actions: []string{ "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", - "To clear the leftover warning and its exit 1, remove the reported directory or install the product it belonged to", + "The leftover note is informational: the backup is complete and the run still exits 0 on an otherwise clean night", "An archive taken before this fix is labelled dual, so restoring it on the corrected host reports partial match", "A PVE host with leftover PBS files no longer offers PBS restore categories and no longer stops PBS services", "A dual backup that lost one role exits with the warning code and names the missing role in its manifest", From e6c39549fea199abc9ec4ba035ec65ac0adaf89a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:02:03 +0200 Subject: [PATCH 12/15] deps(deps): bump the minor-updates group across 1 directory with 3 updates (#316) Bumps the minor-updates group with 3 updates in the / directory: [golang.org/x/crypto](https://github.com/golang/crypto), [golang.org/x/term](https://github.com/golang/term) and [golang.org/x/text](https://github.com/golang/text). Updates `golang.org/x/crypto` from 0.56.0 to 0.57.0 - [Commits](https://github.com/golang/crypto/compare/v0.56.0...v0.57.0) Updates `golang.org/x/term` from 0.45.0 to 0.46.0 - [Commits](https://github.com/golang/term/compare/v0.45.0...v0.46.0) Updates `golang.org/x/text` from 0.41.0 to 0.42.0 - [Release notes](https://github.com/golang/text/releases) - [Commits](https://github.com/golang/text/compare/v0.41.0...v0.42.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.57.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: minor-updates - dependency-name: golang.org/x/term dependency-version: 0.46.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: minor-updates - dependency-name: golang.org/x/text dependency-version: 0.42.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: minor-updates ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- go.mod | 10 +++++----- go.sum | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index ed7c0c01..2b26aa07 100644 --- a/go.mod +++ b/go.mod @@ -10,9 +10,9 @@ require ( github.com/Masterminds/semver/v3 v3.5.0 github.com/charmbracelet/colorprofile v0.4.3 github.com/charmbracelet/x/ansi v0.11.8 - golang.org/x/crypto v0.56.0 - golang.org/x/term v0.45.0 - golang.org/x/text v0.41.0 + golang.org/x/crypto v0.57.0 + golang.org/x/term v0.46.0 + golang.org/x/text v0.42.0 ) require ( @@ -30,6 +30,6 @@ require ( github.com/muesli/cancelreader v0.2.2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect - golang.org/x/sync v0.22.0 // indirect - golang.org/x/sys v0.47.0 // indirect + golang.org/x/sync v0.23.0 // indirect + golang.org/x/sys v0.48.0 // indirect ) diff --git a/go.sum b/go.sum index 65734572..5cb171d0 100644 --- a/go.sum +++ b/go.sum @@ -46,15 +46,15 @@ github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= -golang.org/x/crypto v0.56.0 h1:GUh5Ii4J5jtcseSMiRqr1jXCNHoxjeV9Fmekc2oLy6Y= -golang.org/x/crypto v0.56.0/go.mod h1:OMW5y6CY9l38uPLmxU6l6pwcXp1obtLo3e6gT7gQR2I= +golang.org/x/crypto v0.57.0 h1:3ZVCjf8Ggz7zneR/EHRVx68Ctf+2pmIMP2UFhh9cC6M= +golang.org/x/crypto v0.57.0/go.mod h1:Fdz0i5U6CoizGwLda9DttjSk6qlZo25zYNtR+ycvuZA= golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= -golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= -golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= -golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= -golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= -golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= -golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= -golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= +golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= +golang.org/x/sync v0.23.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= +golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= +golang.org/x/sys v0.48.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= +golang.org/x/term v0.46.0 h1:3+OXuTbaKDgwk8jTi3aSLHRlmWqHEUDUtxnbFigO4YE= +golang.org/x/term v0.46.0/go.mod h1:+K02xbkittuwc0Am4abfA3Fc+XRGXkvBXNO88NCXPoc= +golang.org/x/text v0.42.0 h1:JbOZXgfeCPU9gacVtYliJqOhD+zhrEqK4LfdpmlUZqI= +golang.org/x/text v0.42.0/go.mod h1:ojzP1Z+2QtioaF8DTtO8K5q7JWVVYwZKenzujK0Zd0E= From 54548434714eab9a3c15aa441419473d7aa86461 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:02:27 +0200 Subject: [PATCH 13/15] ci: bump the actions-updates group across 1 directory with 4 updates (#321) Bumps the actions-updates group with 4 updates in the / directory: [codecov/codecov-action](https://github.com/codecov/codecov-action), [github/codeql-action/init](https://github.com/github/codeql-action), [github/codeql-action/analyze](https://github.com/github/codeql-action) and [github/codeql-action/upload-sarif](https://github.com/github/codeql-action). Updates `codecov/codecov-action` from 7.0.0 to 7.1.1 - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/codecov/codecov-action/compare/fb8b3582c8e4def4969c97caa2f19720cb33a72f...303a32d7a59b442fa8d48b6a1cc6825c09c847a5) Updates `github/codeql-action/init` from 4.37.9 to 4.38.1 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/github/codeql-action/compare/cdf488f595d80d6e07e03d4674febd5ab45fa938...1c5b675653bb5c22dbe9b12b556ec555138e09fd) Updates `github/codeql-action/analyze` from 4.37.9 to 4.38.1 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/github/codeql-action/compare/cdf488f595d80d6e07e03d4674febd5ab45fa938...1c5b675653bb5c22dbe9b12b556ec555138e09fd) Updates `github/codeql-action/upload-sarif` from 4.37.9 to 4.38.1 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/github/codeql-action/compare/cdf488f595d80d6e07e03d4674febd5ab45fa938...1c5b675653bb5c22dbe9b12b556ec555138e09fd) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 7.1.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates - dependency-name: github/codeql-action/init dependency-version: 4.38.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates - dependency-name: github/codeql-action/analyze dependency-version: 4.38.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates - dependency-name: github/codeql-action/upload-sarif dependency-version: 4.38.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-updates ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/codecov.yml | 2 +- .github/workflows/codeql.yml | 4 ++-- .github/workflows/security-ultimate.yml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/codecov.yml b/.github/workflows/codecov.yml index 856fbcd0..4fed7bc3 100644 --- a/.github/workflows/codecov.yml +++ b/.github/workflows/codecov.yml @@ -39,7 +39,7 @@ jobs: go test $(go list ./... | grep -v -E '/cmd/|/pbs$|/bech32$|^github.com/tis24dev/proxsave$') -coverprofile=coverage.out - name: Upload coverage reports to Codecov - uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v6 + uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: token: ${{ secrets.CODECOV_TOKEN }} files: coverage.out diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 232960fb..24328d06 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -36,7 +36,7 @@ jobs: cache: false - name: Initialize CodeQL - uses: github/codeql-action/init@cdf488f595d80d6e07e03d4674febd5ab45fa938 + uses: github/codeql-action/init@1c5b675653bb5c22dbe9b12b556ec555138e09fd with: languages: go @@ -45,6 +45,6 @@ jobs: run: go build ./... - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@cdf488f595d80d6e07e03d4674febd5ab45fa938 + uses: github/codeql-action/analyze@1c5b675653bb5c22dbe9b12b556ec555138e09fd with: category: "/language:go" diff --git a/.github/workflows/security-ultimate.yml b/.github/workflows/security-ultimate.yml index 248a9a23..c6b3608d 100644 --- a/.github/workflows/security-ultimate.yml +++ b/.github/workflows/security-ultimate.yml @@ -94,7 +94,7 @@ jobs: # UPLOAD SARIF ######################################## - name: Upload GoSec SARIF - uses: github/codeql-action/upload-sarif@cdf488f595d80d6e07e03d4674febd5ab45fa938 + uses: github/codeql-action/upload-sarif@1c5b675653bb5c22dbe9b12b556ec555138e09fd with: sarif_file: gosec.sarif From dffc261e27a883f09dbe5601a7539e119649a86c Mon Sep 17 00:00:00 2001 From: tis24dev Date: Mon, 21 Sep 2026 15:44:55 +0200 Subject: [PATCH 14/15] fix: an unreadable dpkg status stops counting as proof a package is absent dpkgPackageInstalled answers false for two different facts - the package is absent, or the status file could not be read at all - and the marker table printed "not installed" for both. On a SYSTEM_ROOT_PREFIX mount that carries no /var/lib/dpkg/status, that line stated as checked something the run had never been able to check, and the marker table is the whole explanation an operator gets for a detection verdict. markerLines now reads the status file once and separates the two: a read error prints "not proven installed ()", naming the path the read failed on, and "not installed" keeps meaning exactly that. dpkgPackageInstalled is left alone - it has three callers and two of them are the detection ladder, which is not what this fixes. Also realigns the DetectBackupType sample in RESTORE_TECHNICAL.md with the function it documents. The prose above it already described subtracting incomplete roles; the code block still showed the body from before that change, in a file that declares itself the source of truth for restore compatibility. Both found by reviewers on the v0.39.0 release PR (#322). --- docs/RESTORE_TECHNICAL.md | 16 +++++++++++++--- internal/environment/detect.go | 14 ++++++++++++-- internal/environment/detect_residue_test.go | 19 +++++++++++++++++++ internal/whatsnew/registry.go | 1 + 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/docs/RESTORE_TECHNICAL.md b/docs/RESTORE_TECHNICAL.md index 7027a389..1308bae8 100644 --- a/docs/RESTORE_TECHNICAL.md +++ b/docs/RESTORE_TECHNICAL.md @@ -573,11 +573,21 @@ against a dual host instead of clearing the check as though nothing were missing ```go func DetectBackupType(manifest *backup.Manifest) SystemType { - if len(manifest.ProxmoxTargets) > 0 { - return parseSystemTargets(manifest.ProxmoxTargets) + if manifest == nil { + return SystemTypeUnknown + } + // Declared targets minus every role recorded in incomplete_targets. + if targets := completedTargets(manifest); len(targets) > 0 { + return parseSystemTargets(targets) + } + if len(manifest.ProxmoxTargets) > 0 && len(manifest.IncompleteTargets) > 0 { + // Every declared target failed: the archive carries no role payload at all. + return SystemTypeUnknown } if manifest.ProxmoxType != "" { - return parseSystemTypeString(manifest.ProxmoxType) + if backupType := parseSystemTypeString(manifest.ProxmoxType); backupType != SystemTypeUnknown { + return backupType + } } // Fallback: hostname heuristics return SystemTypeUnknown diff --git a/internal/environment/detect.go b/internal/environment/detect.go index e1c78908..9e53270e 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -1015,10 +1015,20 @@ func markerLines() []string { // host in issue #315 the table listed nine PBS markers and silently omitted the one // that said proxmox-backup-server is NOT installed, so the decisive evidence read as // a check that had never run. Every other marker here reports YES or NO. + // dpkgPackageInstalled answers false for two different facts: the package is absent, + // or the status file could not be read at all. Printing "not installed" for both + // states as proven something the run never managed to check - the case being a + // SYSTEM_ROOT_PREFIX mount that carries no /var/lib/dpkg/status. Reading the file + // once here separates them, so "not installed" keeps meaning exactly that. + _, dpkgReadErr := readFileFunc(resolveUnderPrefix(dpkgStatusFile)) for _, pkg := range []string{"pve-manager", "proxmox-backup-server"} { - if version, ok := dpkgPackageInstalled(pkg); ok { + version, ok := dpkgPackageInstalled(pkg) + switch { + case ok: add("dpkg %s: installed (%s)", pkg, version) - } else { + case dpkgReadErr != nil: + add("dpkg %s: not proven installed (%v)", pkg, dpkgReadErr) + default: add("dpkg %s: not installed", pkg) } } diff --git a/internal/environment/detect_residue_test.go b/internal/environment/detect_residue_test.go index b3650375..4c7f2d25 100644 --- a/internal/environment/detect_residue_test.go +++ b/internal/environment/detect_residue_test.go @@ -425,3 +425,22 @@ func TestThePBSHalfOfTheVersionlessCommandFix(t *testing.T) { t.Fatalf("PBSVersion = %q, want 4.2.0-1 recovered from dpkg", info.PBSVersion) } } + +// An unreadable dpkg status file is not evidence that a package is absent. The table +// used to print "not installed" for it, which reads as a checked fact rather than as +// a check that never ran, and on a SYSTEM_ROOT_PREFIX mount without /var/lib/dpkg/status +// that is the whole explanation an operator gets for the verdict. +func TestMarkerTableDoesNotCallAnUnreadableDpkgStatusProofOfAbsence(t *testing.T) { + root := t.TempDir() // no var/lib/dpkg/status under it at all + + joined := strings.Join(MarkerSnapshot(DetectOptions{RootPrefix: root}), "\n") + for _, pkg := range []string{"pve-manager", "proxmox-backup-server"} { + want := "dpkg " + pkg + ": not proven installed (" + if !strings.Contains(joined, want) { + t.Fatalf("snapshot missing %q:\n%s", want, joined) + } + if strings.Contains(joined, "dpkg "+pkg+": not installed") { + t.Fatalf("snapshot still claims %s is not installed without having read dpkg:\n%s", pkg, joined) + } + } +} diff --git a/internal/whatsnew/registry.go b/internal/whatsnew/registry.go index d93c0b6d..d33c7d54 100644 --- a/internal/whatsnew/registry.go +++ b/internal/whatsnew/registry.go @@ -209,6 +209,7 @@ var notes = []Note{ "Restore reads the host type from the same check the backup uses, instead of keeping a weaker one of its own", "On a PVE plus PBS host, one role failing no longer discards the other role and the shared system payload", "A slow pveversion no longer costs the run its version number: the version is read from the package instead", + "The marker table stops calling a package not installed when it could not read the dpkg status file at all", }, Actions: []string{ "If a run used to fail with failed to get PBS version on a host without PBS, it now completes as a PVE backup", From 8c939a3fbd3a0b8ce21e4f6b158ec0421830eaf9 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Mon, 21 Sep 2026 16:03:28 +0200 Subject: [PATCH 15/15] fix: the dpkg verdict comes from the read that was checked, not from a later one The previous commit read the status file once to tell "absent" from "could not read", then called dpkgPackageInstalled per package - which reads the file again. Three reads, and the verdict printed came from a different read than the one the check was based on. A status file that stopped being readable after the check would have been reported as a package simply not installed: exactly the claim the check exists to stop making. markerLines now reads once and classifies both packages against that data. dpkgPackageInstalled keeps its signature, so detectPVE and detectPBS - the other two callers, and the detection ladder itself - are untouched; its parse half moves into dpkgPackageInstalledIn, which is what markerLines calls. TestMarkerTableClassifiesBothPackagesFromOneDpkgRead serves the status file once and refuses every later read of it. Against the previous commit it fails with "status exists: YES" directly above "dpkg pve-manager: not installed", on a host where pve-manager is installed. Also completes the DetectBackupType sample in RESTORE_TECHNICAL.md: the comment said hostname heuristics and the body returned SystemTypeUnknown without them, while the real function does check the hostname. Both found by CodeRabbit on the v0.39.0 release PR (#322). --- docs/RESTORE_TECHNICAL.md | 9 +++++ internal/environment/detect.go | 36 ++++++++++++------- internal/environment/detect_residue_test.go | 40 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/docs/RESTORE_TECHNICAL.md b/docs/RESTORE_TECHNICAL.md index 1308bae8..b77c5ef0 100644 --- a/docs/RESTORE_TECHNICAL.md +++ b/docs/RESTORE_TECHNICAL.md @@ -590,6 +590,15 @@ func DetectBackupType(manifest *backup.Manifest) SystemType { } } // Fallback: hostname heuristics + if manifest.Hostname != "" { + hostname := strings.ToLower(manifest.Hostname) + if strings.Contains(hostname, "pve") { + return SystemTypePVE + } + if strings.Contains(hostname, "pbs") { + return SystemTypePBS + } + } return SystemTypeUnknown } ``` diff --git a/internal/environment/detect.go b/internal/environment/detect.go index 9e53270e..01310b66 100644 --- a/internal/environment/detect.go +++ b/internal/environment/detect.go @@ -674,6 +674,14 @@ func dpkgPackageInstalled(pkg string) (string, bool) { if err != nil { return "", false } + return dpkgPackageInstalledIn(data, pkg) +} + +// dpkgPackageInstalledIn is the parse half of dpkgPackageInstalled, split out so a +// caller holding the status file can classify several packages against the ONE read +// that produced its verdict. Re-reading per package would let a status file that +// became unreadable between reads come back as a clean "not installed". +func dpkgPackageInstalledIn(data []byte, pkg string) (string, bool) { for _, stanza := range strings.Split(string(data), "\n\n") { if dpkgStanzaField(stanza, "Package") != pkg { continue @@ -1015,20 +1023,24 @@ func markerLines() []string { // host in issue #315 the table listed nine PBS markers and silently omitted the one // that said proxmox-backup-server is NOT installed, so the decisive evidence read as // a check that had never run. Every other marker here reports YES or NO. - // dpkgPackageInstalled answers false for two different facts: the package is absent, - // or the status file could not be read at all. Printing "not installed" for both - // states as proven something the run never managed to check - the case being a - // SYSTEM_ROOT_PREFIX mount that carries no /var/lib/dpkg/status. Reading the file - // once here separates them, so "not installed" keeps meaning exactly that. - _, dpkgReadErr := readFileFunc(resolveUnderPrefix(dpkgStatusFile)) + // A package probe answers false for two different facts: the package is absent, or + // the status file could not be read at all. Printing "not installed" for both states + // as proven something the run never managed to check - the case being a + // SYSTEM_ROOT_PREFIX mount that carries no /var/lib/dpkg/status. + // + // The file is read ONCE and both packages are classified against that read. Calling + // dpkgPackageInstalled per package would read it again each time, so a status file + // that stopped being readable after the check above would be reported as a package + // that is simply not installed - the very claim this is here to stop making. + dpkgStatus, dpkgReadErr := readFileFunc(resolveUnderPrefix(dpkgStatusFile)) for _, pkg := range []string{"pve-manager", "proxmox-backup-server"} { - version, ok := dpkgPackageInstalled(pkg) - switch { - case ok: - add("dpkg %s: installed (%s)", pkg, version) - case dpkgReadErr != nil: + if dpkgReadErr != nil { add("dpkg %s: not proven installed (%v)", pkg, dpkgReadErr) - default: + continue + } + if version, ok := dpkgPackageInstalledIn(dpkgStatus, pkg); ok { + add("dpkg %s: installed (%s)", pkg, version) + } else { add("dpkg %s: not installed", pkg) } } diff --git a/internal/environment/detect_residue_test.go b/internal/environment/detect_residue_test.go index 4c7f2d25..060c63a7 100644 --- a/internal/environment/detect_residue_test.go +++ b/internal/environment/detect_residue_test.go @@ -444,3 +444,43 @@ func TestMarkerTableDoesNotCallAnUnreadableDpkgStatusProofOfAbsence(t *testing.T } } } + +// The marker table classifies both packages against ONE read of the dpkg status file. +// Re-reading per package reopens the very hole the "not proven installed" line closes: +// a status file that stops being readable after the first read would come back as a +// package that is simply not installed, which is a claim the run cannot support. The +// stub here serves the file once and refuses every later read of it. +func TestMarkerTableClassifiesBothPackagesFromOneDpkgRead(t *testing.T) { + root := t.TempDir() + statusPath := filepath.Join(root, "var/lib/dpkg/status") + writeFile(t, statusPath, dpkgStanza("pve-manager", "9.2.18")) + + status, err := os.ReadFile(statusPath) + if err != nil { + t.Fatal(err) + } + served := 0 + setValue(t, &readFileFunc, func(path string) ([]byte, error) { + if path != statusPath { + return os.ReadFile(path) + } + served++ + if served > 1 { + return nil, errors.New("dpkg status: refused on purpose after the first read") + } + return status, nil + }) + + joined := strings.Join(MarkerSnapshot(DetectOptions{RootPrefix: root}), "\n") + for _, want := range []string{ + "dpkg pve-manager: installed (9.2.18)", + "dpkg proxmox-backup-server: not installed", + } { + if !strings.Contains(joined, want) { + t.Fatalf("snapshot missing %q after %d dpkg read(s):\n%s", want, served, joined) + } + } + if served != 1 { + t.Fatalf("dpkg status read %d times, want exactly 1: the verdict must come from the read it checked", served) + } +}