From 630d151460d08c94d69eb28384a574e599bc1648 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 1 Aug 2026 13:19:59 +0100 Subject: [PATCH] Repair half-removed installations in the installer update paths A box with the binary still at /opt/pulse/bin/pulse but /etc/pulse, the systemd unit and the /usr/local/bin symlink deleted by hand takes the update path ("Reinstalling version ..."), which assumed the previous install's environment still existed. With auto-updates enabled the run died at setup_auto_updates writing system.json into the missing config dir; without them it printed a success completion while systemctl enable/start had failed with "Unit pulse.service could not be found", softened into the unprivileged-container note (#1663). Both the --version and menu update flows now run setup_directories and recreate the unit file when it is missing (an existing unit stays untouched so user customizations survive normal updates), and start_pulse refuses to report success when the unit does not exist at all. Documented as a deployment-installability contract invariant. --- .../subsystems/deployment-installability.md | 20 +++++ install.sh | 42 ++++++++- scripts/installtests/root_install_sh_test.go | 89 +++++++++++++++++++ 3 files changed, 148 insertions(+), 3 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index e1c1d845e..a598889eb 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -2005,6 +2005,26 @@ is `safe_systemctl daemon-reload`, which returns 0 by design, so an unchecked write reported success over a truncated unit. The staged helper must also be rejected before the swap unless it is non-empty and begins with `#!`. +Every update-shaped flow over an existing box (the `--version` path and the +interactive update action, like the reinstall action before them) must repair +a half-removed installation rather than assume the previous install's +environment survived: the flow runs `setup_directories` before anything +writes into the config dir, and recreates the systemd unit via +`ensure_systemd_service_installed` when +`/etc/systemd/system/.service` is missing — while leaving an +existing unit untouched so operator customizations (e.g. a non-default +`FRONTEND_PORT`) survive normal updates. A box whose operator deleted +`/etc/pulse`, the unit file and the binary symlink but kept +`/opt/pulse/bin/pulse` takes exactly this path ("Reinstalling version ..."), +and previously crashed writing `system.json` into the missing config dir +when auto-updates were enabled, or — without them — printed a success +completion while `systemctl enable/start` had failed with "Unit +pulse.service could not be found" behind the unprivileged-container note +(issue #1663). `start_pulse` must never report success when the unit does +not exist at all: the installer always writes the unit file itself, even +where systemctl cannot run, so a missing unit is a broken installation +rather than an unprivileged-container quirk and must fail the run loudly. + Changes to the generated units must be able to reach already-deployed boxes. A box installed before the sandbox was widened runs the installer from a `pulse-update.service` whose `ReadWritePaths` excludes the helper and unit diff --git a/install.sh b/install.sh index b04d85e89..2c619c76b 100755 --- a/install.sh +++ b/install.sh @@ -4504,6 +4504,18 @@ EOF safe_systemctl daemon-reload } +# Updates keep an existing unit file untouched (it may carry user +# customizations such as FRONTEND_PORT), but a half-removed installation — +# binary still present, unit deleted by hand (#1663) — must get a fresh unit +# or the enable/start below fails with "Unit ... could not be found". +ensure_systemd_service_installed() { + command -v systemctl >/dev/null 2>&1 || return 0 + if [[ ! -f "/etc/systemd/system/${SERVICE_NAME}.service" ]]; then + print_warn "systemd unit ${SERVICE_NAME}.service is missing; recreating it" + install_systemd_service + fi +} + # Tracks whether Pulse was running before an update-time stop, so start_pulse can # guarantee it comes back up afterward instead of silently leaving it stopped # (#1323: on unprivileged LXC the installer's restart can silently fail). @@ -4561,7 +4573,20 @@ ensure_pulse_running_after_update() { start_pulse() { print_info "Starting Pulse..." - + + # A missing unit is a broken installation, not an unprivileged-container + # quirk: "Unit ... could not be found" must never be softened into the + # container notes below and then reported as success (#1663). The + # installer always writes /etc/systemd/system/.service; systemctl + # cat covers units living elsewhere. + if command -v systemctl >/dev/null 2>&1 \ + && [[ ! -f "/etc/systemd/system/${SERVICE_NAME}.service" ]] \ + && ! timeout 5 systemctl cat "${SERVICE_NAME}.service" >/dev/null 2>&1; then + print_error "systemd unit ${SERVICE_NAME}.service does not exist; Pulse was not started" + print_info "Re-run the installer to recreate the service unit" + return 1 + fi + # Try to enable/start service (may fail in unprivileged containers) if ! safe_systemctl enable $SERVICE_NAME; then print_info "Note: systemctl enable failed (common in unprivileged containers)" @@ -4816,8 +4841,14 @@ main() { stop_pulse_for_update create_user download_pulse + # A half-removed installation (binary present, /etc/pulse or the + # unit file deleted, #1663) reaches this update path; recreate + # the environment the steps below assume instead of crashing on + # a missing config dir or reporting success over a missing unit. + setup_directories setup_update_command - + ensure_systemd_service_installed + # Setup auto-updates if requested if [[ "$ENABLE_AUTO_UPDATES" == "true" ]]; then setup_auto_updates @@ -5018,8 +5049,13 @@ main() { stop_pulse_for_update create_user download_pulse + # Same repair as the --version path: a half-removed + # installation (#1663) must get its config dir and unit + # file back before auto-update setup and start. + setup_directories setup_update_command - + ensure_systemd_service_installed + # Setup auto-updates if requested during update; otherwise # refresh assets a previous install already put in place so a # stale helper (e.g. v5-pinned) doesn't survive the upgrade diff --git a/scripts/installtests/root_install_sh_test.go b/scripts/installtests/root_install_sh_test.go index c0117baff..5c3e58c92 100644 --- a/scripts/installtests/root_install_sh_test.go +++ b/scripts/installtests/root_install_sh_test.go @@ -2035,3 +2035,92 @@ func TestRootInstallScriptRepairAutoUpdateUnitsEntryPoint(t *testing.T) { } assertNoAutoUpdateStagingLitter(t, filepath.Dir(autoUpdateDest), filepath.Dir(servicePath)) } + +// Regression tests for #1663: a half-removed installation (binary still at +// /opt/pulse/bin/pulse, but /etc/pulse and the systemd unit deleted by hand) +// re-ran the installer, which took the update path ("Reinstalling version +// ..."). With auto-updates enabled it crashed writing system.json into the +// missing config dir; without them it printed a success completion while +// `systemctl enable/start` had failed with "Unit pulse.service could not be +// found", softened into the unprivileged-container note. The update flows +// must recreate the config dir and the unit, and start_pulse must fail +// loudly when the unit does not exist at all. +func TestRootInstallScriptUpdateFlowsRepairHalfRemovedInstall(t *testing.T) { + content, err := os.ReadFile(filepath.Join("..", "..", "install.sh")) + if err != nil { + t.Fatalf("read root install.sh: %v", err) + } + + wired := regexp.MustCompile(`(?m)^\s*download_pulse\n(?:\s*#[^\n]*\n)*\s*setup_directories\n\s*setup_update_command\n\s*ensure_systemd_service_installed$`) + if got := len(wired.FindAll(content, -1)); got != 2 { + t.Fatalf("expected both update flows (--version and menu update) to run setup_directories and ensure_systemd_service_installed after download_pulse, found %d", got) + } +} + +func TestRootInstallEnsureSystemdServiceRecreatesMissingUnit(t *testing.T) { + script := ` +set -euo pipefail +SERVICE_NAME="pulse-missing-unit-1663" +print_warn() { echo "WARN: $*"; } +systemctl() { return 0; } +install_systemd_service() { echo "INSTALL_SYSTEMD_SERVICE_CALLED"; } +` + extractRootInstallShellFunction(t, "ensure_systemd_service_installed") + ` +ensure_systemd_service_installed +` + out, err := exec.Command("bash", "-c", script).CombinedOutput() + if err != nil { + t.Fatalf("ensure_systemd_service_installed failed: %v\n%s", err, out) + } + if !strings.Contains(string(out), "INSTALL_SYSTEMD_SERVICE_CALLED") { + t.Fatalf("missing unit was not recreated:\n%s", out) + } +} + +func TestRootInstallStartPulseFailsWhenUnitMissing(t *testing.T) { + stubs := ` +set -euo pipefail +PULSE_WAS_ACTIVE="false" +print_info() { echo "INFO: $*"; } +print_error() { echo "ERROR: $*"; } +print_success() { echo "SUCCESS: $*"; } +safe_systemctl() { return 0; } +timeout() { shift; "$@"; } +sleep() { :; } +journalctl() { return 0; } +ensure_pulse_running_after_update() { return 0; } +` + extractRootInstallShellFunction(t, "start_pulse") + "\n" + + // Unit genuinely absent: no unit file and `systemctl cat` cannot find it. + // start_pulse must fail with a clear error instead of reporting success + // behind the unprivileged-container note. + missing := stubs + ` +SERVICE_NAME="pulse-missing-unit-1663" +systemctl() { return 1; } +start_pulse +` + out, err := exec.Command("bash", "-c", missing).CombinedOutput() + if err == nil { + t.Fatalf("start_pulse reported success with no unit installed:\n%s", out) + } + if !strings.Contains(string(out), "does not exist") { + t.Fatalf("start_pulse did not explain the missing unit:\n%s", out) + } + if strings.Contains(string(out), "SUCCESS:") { + t.Fatalf("start_pulse printed success for a missing unit:\n%s", out) + } + + // Unit resolvable via systemctl cat: the guard must fall through and the + // normal start path must succeed. + present := stubs + ` +SERVICE_NAME="pulse-missing-unit-1663" +systemctl() { return 0; } +start_pulse +` + out, err = exec.Command("bash", "-c", present).CombinedOutput() + if err != nil { + t.Fatalf("start_pulse failed with a resolvable unit: %v\n%s", err, out) + } + if !strings.Contains(string(out), "SUCCESS:") { + t.Fatalf("start_pulse did not report a successful start:\n%s", out) + } +}