From 9db25ba60ebebf8bb965704813c6cd92f764ea68 Mon Sep 17 00:00:00 2001 From: "courtmanr@gmail.com" Date: Tue, 28 Jul 2026 10:57:30 +0100 Subject: [PATCH] Fix doubled auto-update schedule and unfreeze sandboxed refreshes Residual auto-update defects found while triaging #1643 and #1637 (the primary regression was fixed in 806cbe83d): - The generated pulse-update.timer carried both OnCalendar=daily and OnCalendar=02:00, so with RandomizedDelaySec=4h every box attempted two updates per day (00:00-04:00 and 02:00-06:00 windows). Keep the single documented 02:00 schedule. - The generated pulse-update.service sandbox (ProtectSystem=strict) excluded the helper and unit directories from ReadWritePaths, so the unattended path could never refresh /usr/local/bin/pulse-auto-update.sh or rewrite the units - updater fixes only reached boxes via manual installs. Grant the sandbox write access to both directories on purpose. - Because the unattended path replaces the helper bash is currently executing, stage the new helper next to its destination and swap it in with an atomic rename only after repo configuration succeeds. A failed download or configure now leaves the previously working helper in place instead of rm -f'ing it out from under the enabled timer's ExecStart. - Delete scripts/systemd/pulse-update.{service,timer}: orphaned reference copies that had drifted from the units install.sh actually generates and were referenced by nothing. Co-Authored-By: Claude Fable 5 --- .../subsystems/deployment-installability.md | 21 +- install.sh | 47 ++++- scripts/installtests/root_install_sh_test.go | 193 ++++++++++++++++++ scripts/systemd/pulse-update.service | 35 ---- scripts/systemd/pulse-update.timer | 19 -- 5 files changed, 249 insertions(+), 66 deletions(-) delete mode 100644 scripts/systemd/pulse-update.service delete mode 100644 scripts/systemd/pulse-update.timer 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