From 9d4f6ae9232d6d4c53cee8e196cb08bddc00a1b7 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 6 Aug 2026 15:43:26 +0100 Subject: [PATCH] fix(install): stop the agent version warning firing on correct installs The installer compares the agent binary it downloaded against the server that served it, stripping a leading "v" so "v6.0.4" and "6.0.4" match. It did not strip semver build metadata, so a server built from a working tree reporting "6.2.0-rc.8+git.46.g98a638e00.dirty" never matched the "v6.2.0-rc.8" agent it had just served, and the mismatch warning fired on every correct development install. This is the warning's whole job, so a false positive is expensive. It is the only client-side signal that a stale agent was downloaded, and because it always fired it read as background noise. That is exactly how a genuinely stale v6.0.5 agent was installed on a live host earlier today: the warning was there, above the install output, and looked like the one that always appears. Strip build metadata from both sides before comparing, keeping the prerelease suffix because 6.2.0-rc.8 and 6.2.0 are genuinely different releases. This is the same release-identity reduction the server applies when deciding whether a local agent artifact is fresh enough to serve; the contracts now state that one definition governs both ends rather than leaving each side to invent its own. Guarded by a test that pins both normalisation steps and exercises the comparison across the cases that matter: the dev-server shape that used to warn wrongly, the stale-download shape that must still warn, and a prerelease against its release. Verified to fail when either strip is removed. --- .../v6/internal/subsystems/agent-lifecycle.md | 10 ++++ .../subsystems/deployment-installability.md | 11 +++- scripts/install.sh | 10 ++++ scripts/installtests/install_sh_test.go | 52 +++++++++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index e8acbe088..a074ff067 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -110,6 +110,16 @@ server's own agent version, and resolves that expected version from the same source the agent build stamps in rather than from a compiled-in build placeholder, because development builds carry placeholders no version parser accepts and those are precisely the builds whose local artifacts go stale. +One definition of "same agent version" governs both ends of that exchange. It +is the release identity: a leading `v` and semver build metadata are not part +of it, the prerelease suffix is. The server applies it when deciding whether a +local artifact is fresh enough to serve, and the installer applies it when +reporting whether the agent it downloaded matches the server that served it. +Letting the two ends disagree costs more than a redundant check, because the +installer's report is the only signal a human sees: when it compared raw +strings it contradicted a correct server on every development install, and a +contradiction that appears every time is read as noise rather than as the one +warning that mattered. Mock mode is a clean room on the report-admission boundary. Mock mode already suspends pull-based PVE/PBS/PMG collection by never building those clients, and diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index e61aaaa64..18d7345c2 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -2809,7 +2809,16 @@ placeholders that no version parser accepts and those are exactly the builds whose artifacts go stale. Refusal is loud rather than silent: a development server answers 404 naming the stale path and the build command, and a published release falls through to the release-asset proxy and fetches the matching -version. Token-bearing +version. +The installer's own version diagnostics answer to the same identity. When it +compares the agent it downloaded against the server that served it, it compares +release identity, stripping a leading `v` and semver build metadata while +keeping the prerelease suffix, because a server built from a working tree +reports metadata the agent never carries. Comparing raw strings made the +mismatch warning fire on every correct development install, and a warning that +fires when nothing is wrong is worse than no warning: it is the only +client-side signal that a stale agent was downloaded, and one that cries wolf +gets skipped the time it is real. Token-bearing copy-paste commands must pass credentials through ephemeral `--token-file` transport and leave the installed service configured with the persistent runtime token file, never a raw `--token` process argument. diff --git a/scripts/install.sh b/scripts/install.sh index 7fad81cef..4f0581dab 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -3628,8 +3628,18 @@ NEW_VERSION=$("$TMP_BIN" --version 2>/dev/null | head -1 || echo "unknown") # Compare versions with any leading "v" stripped so the agent binary's "v6.0.4" # and the server /api/version "6.0.4" are treated as equal. Only a genuine # version difference (e.g. 6.0.3 vs 6.0.4) should raise the mismatch warning. +# +# Semver build metadata is stripped for the same reason. A server built from a +# working tree reports "6.2.0-rc.8+git.46.g98a638e00.dirty" while the agent it +# serves carries the release identity "v6.2.0-rc.8"; those are the same release, +# and comparing them raw made this warning fire on every correct development +# install. A warning that fires when nothing is wrong is worse than no warning, +# because it trains the reader to skip the one time it is real. The prerelease +# suffix is deliberately kept: 6.2.0-rc.8 and 6.2.0 are genuinely different. NEW_VERSION_NORMALIZED="${NEW_VERSION#v}" +NEW_VERSION_NORMALIZED="${NEW_VERSION_NORMALIZED%%+*}" SERVER_VERSION_NORMALIZED="${SERVER_VERSION#v}" +SERVER_VERSION_NORMALIZED="${SERVER_VERSION_NORMALIZED%%+*}" if [[ -n "$SERVER_VERSION" && -n "$NEW_VERSION" && "$NEW_VERSION" != "unknown" && "$NEW_VERSION_NORMALIZED" != "$SERVER_VERSION_NORMALIZED" ]]; then log_warn "Downloaded agent version (${NEW_VERSION}) does not match Pulse server version (${SERVER_VERSION}). Check that Pulse is upgraded and that any reverse proxy is not serving a stale cached binary." diff --git a/scripts/installtests/install_sh_test.go b/scripts/installtests/install_sh_test.go index 3f1018f5e..587bd59de 100644 --- a/scripts/installtests/install_sh_test.go +++ b/scripts/installtests/install_sh_test.go @@ -5288,3 +5288,55 @@ func TestInstallSHStopsWrapperBeforeAgentInEveryBranch(t *testing.T) { } } } + +// TestInstallSHVersionMismatchWarningIgnoresBuildMetadata pins the comparison +// behind the "Downloaded agent version does not match" warning. A server built +// from a working tree reports build metadata the agent it serves never carries, +// so a raw comparison fired on every correct development install. That is not a +// cosmetic annoyance: the warning is the only client-side signal that a stale +// agent was downloaded, and one that cries wolf gets skipped the time it counts. +func TestInstallSHVersionMismatchWarningIgnoresBuildMetadata(t *testing.T) { + content, err := os.ReadFile(repoFile("scripts", "install.sh")) + if err != nil { + t.Fatalf("read install.sh: %v", err) + } + script := string(content) + + for _, required := range []string{ + `NEW_VERSION_NORMALIZED="${NEW_VERSION_NORMALIZED%%+*}"`, + `SERVER_VERSION_NORMALIZED="${SERVER_VERSION_NORMALIZED%%+*}"`, + } { + if !strings.Contains(script, required) { + t.Errorf("version comparison must strip semver build metadata, missing: %s", required) + } + } + + // Exercise the same normalisation the installer performs. + normalize := func(v string) string { + v = strings.TrimPrefix(v, "v") + if idx := strings.Index(v, "+"); idx >= 0 { + v = v[:idx] + } + return v + } + cases := []struct { + agent string + server string + warns bool + }{ + // The shape that fired on every correct dev install. + {"v6.2.0-rc.8", "6.2.0-rc.8+git.46.g98a638e00.dirty", false}, + {"v6.2.0-rc.8", "6.2.0-rc.8", false}, + // The stale download this warning exists to catch. + {"v6.0.5-54-gc862fb0ca0", "6.2.0-rc.8+git.46.g98a638e00.dirty", true}, + // A prerelease is genuinely not its release. + {"v6.2.0", "6.2.0-rc.8+git.46.gabc.dirty", true}, + {"v6.1.2", "6.2.0-rc.8", true}, + } + for _, tc := range cases { + got := normalize(tc.agent) != normalize(tc.server) + if got != tc.warns { + t.Errorf("agent %q vs server %q: warns=%v, want %v", tc.agent, tc.server, got, tc.warns) + } + } +}