diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index 4d1e93472..3b3c8c8d5 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -1896,8 +1896,25 @@ already-installed auto-update assets unconditionally via helper script and rewriting the units so a version-pinned helper from a previous major (which never selects newer releases and reports "Already running latest version" forever) cannot survive an upgrade — while leaving -`system.json` and the timer's enabled/started state untouched. The -rendered-unit execution, refresh-behavior, and call-site wiring tests in +`system.json` and the timer's enabled/started state untouched. The rendered +timer must schedule exactly one update attempt per day — a single +`OnCalendar` entry at 02:00 with the 4h randomized spread — because a second +`OnCalendar=daily` line silently doubled every box's daily update attempts +(issue #1643). The rendered service sandbox must keep the helper's directory +and the unit directory inside `ReadWritePaths`: unattended updates run +`install.sh` (and therefore `refresh_auto_updates`) under that very sandbox, +and a sandbox that excludes those paths freezes every installed helper and +unit at whatever a manual install last wrote (issue #1637 triage). Because +the unattended path replaces the helper script that bash is currently +executing, `install_auto_update_assets` must stage the new helper in the +destination directory and swap it in with an atomic same-filesystem rename +only after repo configuration succeeds; no failure path may delete or +truncate a previously working helper (the old `rm -f` on configure failure +left the enabled timer with a dangling `ExecStart`). The generated units are +the only unit source — no checked-in reference copies of +`pulse-update.service` / `pulse-update.timer` may exist to drift from the +heredocs. The rendered-unit execution, schedule, sandbox-writability, +failure-preservation, refresh-behavior, and call-site wiring tests in `scripts/installtests/root_install_sh_test.go` are the owned proof surface for these invariants. That same server-installer uninstall must also leave no legacy companion diff --git a/install.sh b/install.sh index d7ea1f13c..580e4a956 100755 --- a/install.sh +++ b/install.sh @@ -4080,24 +4080,46 @@ install_auto_update_assets() { local update_timer_path="${UPDATE_TIMER_PATH:-${PULSE_UPDATE_TIMER_PATH:-/etc/systemd/system/${service_name}-update.timer}}" local update_timer_unit update_timer_unit="$(basename "$update_timer_path")" + local auto_update_bin_dir update_unit_dir update_timer_dir + auto_update_bin_dir="$(dirname "$auto_update_dest")" + update_unit_dir="$(dirname "$update_service_path")" + update_timer_dir="$(dirname "$update_timer_path")" + local unit_write_dirs="$update_unit_dir" + if [[ "$update_timer_dir" != "$update_unit_dir" ]]; then + unit_write_dirs="$unit_write_dirs $update_timer_dir" + fi - # Copy auto-update script if it exists in the release + # Stage the helper next to its destination and only swap it in once it is + # fully configured. During unattended updates install.sh runs under the + # pulse-update.service sandbox, where the file being replaced is the very + # script bash is executing — the same-directory mv keeps the swap an + # atomic rename — and a failed download or configure must leave the + # previously working helper in place rather than deleting it out from + # under the timer's ExecStart. + local staged_helper + if ! staged_helper=$(mktemp "${auto_update_dest}.staged.XXXXXX"); then + print_warn "Cannot write to ${auto_update_bin_dir} to stage the auto-update helper." + return 1 + fi if [[ -f "$install_dir/scripts/pulse-auto-update.sh" ]]; then - cp "$install_dir/scripts/pulse-auto-update.sh" "$auto_update_dest" - chmod +x "$auto_update_dest" + # Copy auto-update script if it exists in the release + cp "$install_dir/scripts/pulse-auto-update.sh" "$staged_helper" else print_info "Downloading auto-update script..." - if ! download_auto_update_script; then + if ! AUTO_UPDATE_DEST="$staged_helper" download_auto_update_script; then print_warn "Could not download the auto-update helper after multiple attempts." + rm -f "$staged_helper" return 1 fi fi - if ! configure_auto_update_script_repo "$auto_update_dest"; then + if ! configure_auto_update_script_repo "$staged_helper"; then print_warn "Could not configure the auto-update helper for the selected release repo." - rm -f "$auto_update_dest" + rm -f "$staged_helper" return 1 fi + chmod +x "$staged_helper" + mv "$staged_helper" "$auto_update_dest" # Install systemd timer and service. The heredoc is unquoted so the # installer substitutes paths/names; the rendered unit must contain no @@ -4129,7 +4151,10 @@ Environment="PULSE_UPDATE_TIMER_UNIT=$update_timer_unit" PrivateTmp=yes ProtectHome=yes ProtectSystem=strict -ReadWritePaths=$install_dir $config_dir /tmp +# The helper and unit directories are writable on purpose: the installer this +# service runs refreshes the auto-update helper and rewrites these units, and +# without write access updater fixes only ever reach boxes via manual installs. +ReadWritePaths=$install_dir $config_dir /tmp $auto_update_bin_dir $unit_write_dirs PrivateNetwork=no Nice=10 @@ -4145,8 +4170,10 @@ After=network-online.target Wants=network-online.target [Timer] -OnCalendar=daily -OnCalendar=02:00 +# One update attempt per day, landing in the 02:00-06:00 window. A second +# OnCalendar=daily line here used to double the schedule with an extra +# midnight trigger (issue #1643). +OnCalendar=*-*-* 02:00:00 RandomizedDelaySec=4h Persistent=true AccuracySec=1h @@ -4210,7 +4237,7 @@ setup_auto_updates() { # Start the timer safe_systemctl start "$update_timer_unit" || true - print_success "Automatic updates enabled (daily check with 2-6 hour random delay)" + print_success "Automatic updates enabled (daily check between 02:00 and 06:00)" } # Updates and reinstalls only run setup_auto_updates when the user opts in, diff --git a/scripts/installtests/root_install_sh_test.go b/scripts/installtests/root_install_sh_test.go index 2d632ed50..523d1a80d 100644 --- a/scripts/installtests/root_install_sh_test.go +++ b/scripts/installtests/root_install_sh_test.go @@ -1441,3 +1441,196 @@ echo "SURVIVED_EXISTING_LINK" t.Fatalf("install_binary_symlink should not invoke ln when the correct link already exists:\n%s", got) } } + +// renderAutoUpdateUnits runs install_auto_update_assets against a seeded +// release helper and returns the rendered service and timer unit contents. +func renderAutoUpdateUnits(t *testing.T) (string, string, string, string) { + t.Helper() + + tmpDir := t.TempDir() + configDir := filepath.Join(tmpDir, "config") + installDir := filepath.Join(tmpDir, "install") + autoUpdateSrc := filepath.Join(installDir, "scripts", "pulse-auto-update.sh") + autoUpdateDest, servicePath, timerPath := prepareAutoUpdatePaths(t, tmpDir) + + if err := os.MkdirAll(configDir, 0755); err != nil { + t.Fatalf("mkdir config dir: %v", err) + } + if err := os.MkdirAll(filepath.Dir(autoUpdateSrc), 0755); err != nil { + t.Fatalf("mkdir auto-update src dir: %v", err) + } + if err := os.WriteFile(autoUpdateSrc, []byte("#!/usr/bin/env bash\n"), 0755); err != nil { + t.Fatalf("write auto-update src: %v", err) + } + + script := ` + CONFIG_DIR="` + configDir + `" + INSTALL_DIR="` + installDir + `" + PULSE_AUTO_UPDATE_DEST="` + autoUpdateDest + `" + PULSE_UPDATE_SERVICE_PATH="` + servicePath + `" + PULSE_UPDATE_TIMER_PATH="` + timerPath + `" + GITHUB_REPO="rcourtman/Pulse" + print_info() { :; } + print_warn() { :; } + safe_systemctl() { :; } +` + extractRootInstallShellFunction(t, "repo_web_url") + ` +` + extractRootInstallShellFunction(t, "configure_auto_update_script_repo") + ` +` + extractRootInstallShellFunction(t, "install_auto_update_assets") + ` + install_auto_update_assets + ` + + out, err := exec.Command("bash", "-c", script).CombinedOutput() + if err != nil { + t.Fatalf("bash: %v\n%s", err, out) + } + + serviceBytes, err := os.ReadFile(servicePath) + if err != nil { + t.Fatalf("read rendered service unit: %v", err) + } + timerBytes, err := os.ReadFile(timerPath) + if err != nil { + t.Fatalf("read rendered timer unit: %v", err) + } + return string(serviceBytes), string(timerBytes), autoUpdateDest, servicePath +} + +// Regression test for the doubled auto-update schedule (issue #1643): the +// rendered timer carried both OnCalendar=daily and OnCalendar=02:00, so with +// RandomizedDelaySec=4h every box attempted two updates per day, one in the +// 00:00-04:00 window and one in 02:00-06:00. Exactly one OnCalendar line may +// survive, and it must be the documented 02:00 schedule. +func TestAutoUpdateTimerSchedulesSingleDailyRun(t *testing.T) { + _, timer, _, _ := renderAutoUpdateUnits(t) + + var schedules []string + for _, line := range strings.Split(timer, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "OnCalendar=") { + schedules = append(schedules, strings.TrimSpace(line)) + } + } + if len(schedules) != 1 { + t.Fatalf("rendered timer must hold exactly one OnCalendar line, got %d:\n%s", len(schedules), timer) + } + if schedules[0] != "OnCalendar=*-*-* 02:00:00" { + t.Fatalf("rendered timer schedule = %q, want the documented 02:00 daily run:\n%s", schedules[0], timer) + } + if !strings.Contains(timer, "RandomizedDelaySec=4h\n") { + t.Fatalf("rendered timer lost the 4h random spread:\n%s", timer) + } +} + +// Regression test for the sandbox that froze updater fixes (issue #1637 +// triage): pulse-update.service runs install.sh with ProtectSystem=strict and +// ReadWritePaths that excluded the helper and unit directories, so +// refresh_auto_updates could never replace /usr/local/bin/pulse-auto-update.sh +// or rewrite the units during an unattended update — helper fixes only reached +// boxes via manual installs. The rendered sandbox must grant write access to +// both directories. +func TestAutoUpdateServiceSandboxAllowsHelperAndUnitRefresh(t *testing.T) { + service, _, autoUpdateDest, servicePath := renderAutoUpdateUnits(t) + + var rwLine string + for _, line := range strings.Split(service, "\n") { + if strings.HasPrefix(line, "ReadWritePaths=") { + rwLine = line + } + } + if rwLine == "" { + t.Fatalf("rendered service unit lost its ReadWritePaths line:\n%s", service) + } + paths := strings.Fields(strings.TrimPrefix(rwLine, "ReadWritePaths=")) + want := map[string]bool{ + filepath.Dir(autoUpdateDest): false, + filepath.Dir(servicePath): false, + } + for _, p := range paths { + if _, ok := want[p]; ok { + want[p] = true + } + } + for dir, found := range want { + if !found { + t.Fatalf("ReadWritePaths %q is missing %q; unattended refreshes cannot write there:\n%s", rwLine, dir, service) + } + } +} + +// Regression test for the destructive failure path in +// install_auto_update_assets (issue #1637 triage): a failed +// configure_auto_update_script_repo used to rm -f the installed helper, +// leaving the still-enabled timer with a dangling ExecStart. The helper is now +// staged in its destination directory and swapped in with an atomic rename +// only after configuration succeeds, so any failure must leave the previously +// working helper untouched, the units unwritten, and no staging litter behind. +func TestInstallAutoUpdateAssetsKeepsWorkingHelperWhenConfigureFails(t *testing.T) { + tmpDir := t.TempDir() + configDir := filepath.Join(tmpDir, "config") + installDir := filepath.Join(tmpDir, "install") + autoUpdateSrc := filepath.Join(installDir, "scripts", "pulse-auto-update.sh") + autoUpdateDest, servicePath, timerPath := prepareAutoUpdatePaths(t, tmpDir) + + if err := os.MkdirAll(configDir, 0755); err != nil { + t.Fatalf("mkdir config dir: %v", err) + } + if err := os.MkdirAll(filepath.Dir(autoUpdateSrc), 0755); err != nil { + t.Fatalf("mkdir auto-update src dir: %v", err) + } + if err := os.WriteFile(autoUpdateSrc, []byte("#!/usr/bin/env bash\necho new-helper\n"), 0755); err != nil { + t.Fatalf("write auto-update src: %v", err) + } + workingHelper := "#!/usr/bin/env bash\necho working-helper\n" + if err := os.WriteFile(autoUpdateDest, []byte(workingHelper), 0755); err != nil { + t.Fatalf("write installed helper: %v", err) + } + + // The failing configure stub is defined after the extracted functions so + // it overrides the real implementation. + script := ` + CONFIG_DIR="` + configDir + `" + INSTALL_DIR="` + installDir + `" + PULSE_AUTO_UPDATE_DEST="` + autoUpdateDest + `" + PULSE_UPDATE_SERVICE_PATH="` + servicePath + `" + PULSE_UPDATE_TIMER_PATH="` + timerPath + `" + GITHUB_REPO="rcourtman/Pulse" + print_info() { :; } + print_warn() { :; } + safe_systemctl() { :; } +` + extractRootInstallShellFunction(t, "repo_web_url") + ` +` + extractRootInstallShellFunction(t, "install_auto_update_assets") + ` + configure_auto_update_script_repo() { return 1; } + if install_auto_update_assets; then + echo "UNEXPECTED_SUCCESS" + else + echo "FAILED_AS_EXPECTED" + fi + ` + + out, err := exec.Command("bash", "-c", script).CombinedOutput() + if err != nil { + t.Fatalf("bash: %v\n%s", err, out) + } + if !strings.Contains(string(out), "FAILED_AS_EXPECTED") { + t.Fatalf("install_auto_update_assets should report failure when configure fails:\n%s", out) + } + + helper, err := os.ReadFile(autoUpdateDest) + if err != nil { + t.Fatalf("installed helper is gone after failed configure: %v", err) + } + if string(helper) != workingHelper { + t.Fatalf("failed configure replaced the working helper:\n%s", helper) + } + if _, err := os.Stat(servicePath); !os.IsNotExist(err) { + t.Fatalf("failed configure still rewrote the service unit (stat err %v)", err) + } + entries, err := os.ReadDir(filepath.Dir(autoUpdateDest)) + if err != nil { + t.Fatalf("read helper dir: %v", err) + } + for _, entry := range entries { + if strings.Contains(entry.Name(), ".staged.") { + t.Fatalf("failed configure left staging litter behind: %s", entry.Name()) + } + } +} diff --git a/scripts/systemd/pulse-update.service b/scripts/systemd/pulse-update.service deleted file mode 100644 index 71cf98889..000000000 --- a/scripts/systemd/pulse-update.service +++ /dev/null @@ -1,35 +0,0 @@ -[Unit] -Description=Automatic Pulse update check and install -Documentation=https://github.com/rcourtman/Pulse -After=network-online.target -Wants=network-online.target - -[Service] -Type=oneshot -# Run as root to allow service restart -User=root -Group=root -# Skip auto-update run unless a supported Pulse service is active -ExecCondition=/bin/sh -c 'systemctl is-active --quiet pulse || systemctl is-active --quiet pulse-backend' -# Use the update script -ExecStart=/usr/local/bin/pulse-auto-update.sh -# Restart policy for the update service itself -Restart=no -# Timeout for the update process (10 minutes should be plenty) -TimeoutStartSec=600 -# Log to journal -StandardOutput=journal -StandardError=journal -SyslogIdentifier=pulse-update -# Security hardening -PrivateTmp=yes -ProtectHome=yes -ProtectSystem=strict -ReadWritePaths=/opt/pulse /etc/pulse /tmp -# Network access needed for GitHub -PrivateNetwork=no -# Nice level to run updates at lower priority -Nice=10 - -[Install] -WantedBy=multi-user.target diff --git a/scripts/systemd/pulse-update.timer b/scripts/systemd/pulse-update.timer deleted file mode 100644 index 49af3c9a0..000000000 --- a/scripts/systemd/pulse-update.timer +++ /dev/null @@ -1,19 +0,0 @@ -[Unit] -Description=Daily check for Pulse updates -Documentation=https://github.com/rcourtman/Pulse -After=network-online.target -Wants=network-online.target - -[Timer] -# Run daily at 2 AM with a random delay up to 4 hours -# This spreads the load on GitHub and prevents all instances updating at once -OnCalendar=daily -OnCalendar=02:00 -RandomizedDelaySec=4h -# Persist the last trigger time during downtime -Persistent=true -# Ensure we run if the system was off when scheduled -AccuracySec=1h - -[Install] -WantedBy=timers.target \ No newline at end of file